| | [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #3216
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 3 - Moderate, e.g. unable to compile the software |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | Fl_Secret_Input allows input even if deactivated |
|---|
| Version: | 1.3-current |
|---|
| Created By: | greg.ercolano |
|---|
| Assigned To: | AlbrechtS |
|---|
| Fix Version: | 1.3.4 (SVN: v10774) |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
Trouble Report Comments:
| |
| #1 | greg.ercolano
12:02 Apr 13, 2015 |
| See the summary.
Can replicate by changing the test/input.cxx program
to call deactivate() in the Fl_Secret_Input widget..
doing so, I can still type into the field, even though
it's grayed out.
Also, if I encompass all the widgets with an Fl_Group
and deactivate() the group, only the Secret: field accepts
input, and from there, I can use keynav to navigate to the other
input fields and buttons, and enter text into them..!
It seems that if any widget has a bug like this, other widgets
can be 'infiltrated'.
Entering this as "Unpublished" because this is "kind of"
a security issue, in that fields that are deactivated by the app
can still be manipulated by the user. | |
| |
| #2 | greg.ercolano
12:07 Apr 13, 2015 |
| Attaching patch that shows how to add a deactivated group
around all the widgets, and still be able to type into Secret:,
even though it and everything else is grayed out.
And once you type into Secret:, you can use keynav (Tab key)
to navigate and type into other widgets and push buttons with
keynav (Tab, Spacebar).
Mouse events are ignored properly.
Of interest, if you use this patch to change the code, and then
hide() the Secret widget, then the interface becomes "secure";
you can't type or use keyboard nav to access other widgets..
so this problem seems specific to Fl_Secret_Input, and with this
problem it can 'break' the behavior of other widgets.. | |
| |
| #3 | greg.ercolano
13:41 Apr 13, 2015 |
| OK, it seems the thing that triggers this is /not/ FL_SECRET_INPUT,
it depends on the text you type, and has something to do with shortcuts.
In my case I was always typing "asdf", and apparently the 's' is what
triggered "Secret:", because 's' is the shortcut (its label is "&Secret").
So if you hit 'm' first, then the &Multiline field wakes up.
If you hit 's' first, then the &Secret field wakes up.
Not sure why a bare letter like 's' or 'm' without the Alt modifier
is having this effect, but it's probably a bug in the Fl_Input
event handling not properly checking for the Alt modifier when shortcuts
are processed.. | |
| |
| #4 | AlbrechtS
14:06 Apr 18, 2015 |
| Posted file fl_shortcut_test.patch contains modifications of
- test/input.cxx : updated testfile
- src/Fl.cxx : Debugging statements for FL_KEYBOARD and FL_SHORTCUT events
Use with `patch -p1 fl_shortcut_test.patch'. | |
| |
| #5 | AlbrechtS
14:17 Apr 18, 2015 |
| Uploaded file shortcut_test.txt shows a log of a former version of the modified test program test/input.cxx (see fl_shortcut_test.patch for the latest version).
As discussed in fltk.coredev this log and the comments show how an FL_SHORTCUT event sent to an inactive widget can trigger the bad behavior Greg described.
To see the effect, launch the program and type 's' or 'm' and watch the output to stdout. If you type 's' nothing seems to happen, but if you type 'm' a cursor appears in the inactive multiline input field and you can enter text.
I'm preparing a patch. Will follow up with a proposal probably tomorrow. | |
| |
| #6 | AlbrechtS
08:00 Apr 19, 2015 |
| Attached file fl_shortcut_test_v3.patch contains revised test code and a patch proposal. Use `patch -p1' to patch the FLTK sources, then run test/input and watch the output.
test/input.cxx has a new feature. Edit one line near the end of the file to use the "normal" vs. "inactive group" test scenario:
+#if (0) // edit: (0) or (1) for different test scenarios
+ grp->deactivate();
+#endif
I'll follow up with a test logfile and a patch file that includes only the relevant patch, i.e. no additional test statements.
Note: use svn -r 10708 or later. I renamed the internal (static) send(..) function to send_event(..). Previous patches are obsolete now. | |
| |
| #7 | AlbrechtS
08:11 Apr 19, 2015 |
| Attached file shortcut_test_v3.txt contains the output of both test scenarios:
(1) group is active, only &Secret input is deactivated
(2) both the containing group and &Secret are deactivated
New test features include:
(a) 'belowmouse' indicates that the target widget for the event being sent is the Fl::belowmouse() widget (as it was before, whereas ...)
(b) '<mouse>' indicates that the target widget for the event being sent _contains_ the Fl::belowmouse() widget.
In scenario (1) keys 'r', 's', 't', and 'm' behave as expected (see the final result of send_event(..) = 0 or 1, resp.. After typing 'm' the multiline input is active, keys a,s,d,f go directly to the widget and the callback shows its contents ('asdf').
In scenario (2) all FL_KEYBOARD and FL_SHORTCUT events are sent to the window since the group (a direct child of the window) is set inactive. No widget gets the focus.
This is the expected behavior. | |
| |
| #8 | AlbrechtS
08:24 Apr 19, 2015 |
| Attached file FL_SHORTCUT.patch is the patch that could be applied directly to svn r 10708 or later. No test statements included. Use `patch -p1' to apply.
The only difference WRT the old code is one line:
// Try it as shortcut, sending to mouse widget and all parents:
- wi = belowmouse();
+ // wi = belowmouse();
+ wi = find_active(belowmouse()); // STR #3216
Of course there is also the new function find_active(..).
find_active() needs exactly one trip through the widget hierarchy from the target widget up to the top level window - that's as much as one call to active_r() would need if the widget were active(). active_r() would terminate when it finds an inactive widget (group), whereas find_active() always needs to check all parents up to the top level window.
This is still fast and light and is used only for event delivery when an FL_SHORTCUT event is being sent to the Fl::belowmouse() widget which might be an inactive widget.
Note that the Fl::focus() widget must always be active and thus doesn't need this check.
Please test and confirm that the patch fixes the issue. For further discussion please use fltk.coredev, unless there are results (summaries) that should be posted here. | |
| |
| #10 | greg.ercolano
13:32 Apr 19, 2015 |
| Will test hopefully later today. | |
| |
| #11 | greg.ercolano
07:38 Jun 24, 2015 |
| Patch seems to solve it -- want to commit?
If so, feel free to assign + close.
A few notes:
1. Do you think find_active() would be useful as a public function?
2. Could the code for find_active() perhaps be reduced to just:
static Fl_Widget *find_active(Fl_Widget *wi) {
Fl_Widget *found = 0;
for (; wi; wi = wi->parent()) {
if (wi->active()) {
if (!found) found = wi;
} else {
found = 0;
}
}
return found;
}
(Seemed like the use of 'const' and variable 'o' could be removed)
3) The docs say it returns the 'first active widget'
which I take to mean the first active widget encountered walking up
the hierarchy.
But looking at the code, I'm thinking it may skip the first one it
encounters if ones above it are also deactivated. So if the hierarchy is:
window <-- active
grp1 <-- deactivated *
scroll <-- active
grp2 <-- deactivated *
widget <-- [passed to find_active()]
..the comment seems to imply 'scroll' would be returned, but I'm thinking
it would actually return 'window' in this case, because the function doesn't
break out early on the first encountered, it travels all the way to the top level window.
If so, perhaps the comment can be clarified by adding "highest level", e.g.
// Find the *highest level* first active widget, i.e. one where active_r() would be
// true, starting at the widget wi and walking up the widget hierarchy
// to the top level window. | |
| |
| #12 | AlbrechtS
15:31 Jun 24, 2015 |
| Yes, I'd like to commit the patch ASAP, and I'm glad you found the time to test it. I assigned the STR to me and set it "active".
As to your questions:
(1) Something like that might be useful as a public method, but I think it should wait until after release of 1.3.4 (i.e. 1.4.0). I don't have the time to think about a use case and a proper API (and documentation). Once we have it we can still /use/ it and remove the static function.
(2) Your suggested code change looks good, I'll take a closer look tomorrow. Although I don't think it would make a notable difference in the compiled code. I don't remember why I used this 'const' and 'o' construct - maybe I copied some code and modified it.
(3) Your example is correct, and that is the intended result. The wording might be clarified though. I don't think that "highest level" and "first" should be combined, and *if* then it should IMHO be "lowest level". With "active" I meant "_really active_", and I tried to clarify this with the words "i.e. one where active_r() would be true".
Technically correct (and shorter) would be: "Find the first active_r() widget, starting at the widget wi and walking up the widget hierarchy to the top level window" - but I wanted to avoid the term "active_r()" in a normal sentence.
In this wording "first" should be clear (starting at wi and walking up ...).
Now I think, this could be a better choice, because it describes the intended result /technically/ correct. What do you think? I'm always open for better proposals by native English speakers... | |
| |
| #13 | AlbrechtS
07:21 Jun 27, 2015 |
| Fixed in Subversion repository.
@Greg: I applied your suggestions and removed one set of curly braces in the 'else' clause.
I clarified the comments by using 'active_r()' instead of 'active' etc. where useful. This is an internal comment anyway.
If we used this function for an official API we could change the comments if necessary.
Closed. Please feel free to add comments or reopen. | |
[ Return to Bugs & Features ]
|
| |
