1278552 - Ctrl+F doesn't focus filter field in ruleview in some cases

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160511030221
STR_1 (bad):
1. Open data:text/html,<style>body{color:black}
2. Open devtools -> ruleview, select string "color: black;" in ruleview with mouse, starting from ";"
3. Press Ctrl+F

STR_2 (good):
1. Open data:text/html,<style>body{color:black}
2. Open devtools -> ruleview, select "olo" in string "color: black;" in ruleview with mouse
3. Press Ctrl+F

AR:  STR_1 - field "Search HTML" is focused. STR_2 - field "Filter styles" is focused
ER:  The same action in both cases. Preferably, search field "Filter styles" should become focused

STR_1 was regressed 2 times
[1] - bug 1243598: "everything works OK" -> "filter in computed view becomes focused"
> regression range:   https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=3b8c69102f125cfd6a6e870bfde4c08fb4cfe899&tochange=4f1701beb4ec0db8ccf400fa8ec8fa4f275b76cc

[2] - bug 1260235: "filter in computed view becomes focused" -> "field `Search HTML` becomes focused"
> regression range:   https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8edeb8f0ca56598918f4e5f6b018d5bb989cf133&tochange=2b4d5580c46da0d6e83cf597cbfc3811b64332d0

Comment 1

[Brian Grinstead [:bgrins]]

This is likely a side effect of all the inspector tools now being in a single iframe, and the activeElement not being set inside the rule view in STR_1.

Comment 2

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[Patrick Brosset <:pbro>]

I think I have a simple solution we could land and uplift to 49 and 48. Taking the bug now.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug 1278552 - Make sidebar containers focusable so shortcuts work when they are active;

Review commit: https://reviewboard.mozilla.org/r/64884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64884/

Comment 5

[Patrick Brosset <:pbro>]

This is a first attempt at fixing this.
Perhaps we should have a more generic approach on the long run. If our goal is to reduce even further the number of iframes, then perhaps we could have something at toolbox level that detects user activity and routes shortcuts to the right panel.

Comment 6

[Patrick Brosset <:pbro>]

However, if we want to uplift this to 48, then the simpler the fix, the better.

Comment 7

[Patrick Brosset <:pbro>]

As discussed with Julian, this patch doesn't account for keyboard navigation. So if you tab through the UI, from the markup-view to the rule-view, for instance, then ctrl+F isn't going to focus the rule-view field.

Another approach is to use the focus event. By listening to this event instead, we can know which part of the inspector panel is currently active, but of course that will work only for focusable elements. So the STR in comment 0 wouldn't be fixed. Also, if you just clicked in the middle of the rule-view, on empty space, then that wouldn't count as focusing the panel.

So, yet another approach is to simply make the rule-view focusable with tabindex="0" on the container, so that wether you click anywhere in the container or you access any elements via the keyboard, then the rule-view is the active element, and therefore we know that ctrl+F should focus the rule-view search field.
One drawback of this approach is that it adds one element that you need to go through when tabbing.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8771935 [details]
Bug 1278552 - Make sidebar containers focusable so shortcuts work when they are active;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64884/diff/1-2/

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8771935 [details]
Bug 1278552 - Make sidebar containers focusable so shortcuts work when they are active;

https://reviewboard.mozilla.org/r/64884/#review61890

Looks good, thanks! 
Let's just check with :yzen if the additional tabindex are ok for accessibility.

::: devtools/client/inspector/test/browser_inspector_search-sidebar.js:54
(Diff revision 2)
> +
> +function clickInRuleView(inspector) {
> +  let win = inspector.panelDoc.defaultView;
> +  let el = inspector.panelDoc.querySelector("#sidebar-panel-ruleview");
> +  EventUtils.synthesizeMouseAtCenter(el, {type: "mousedown"}, win);
> +  EventUtils.synthesizeMouseAtCenter(el, {type: "mouseup"}, win);

I think you can use 

  EventUtils.synthesizeMouseAtCenter(el, {}, win);

If you don't define a type, EventUtils will simulate mousedown, then mouseup automatically -> http://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#373

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset <:pbro> from comment #7)
> [...]
> 
> So, yet another approach is to simply make the rule-view focusable with
> tabindex="0" on the container, so that wether you click anywhere in the
> container or you access any elements via the keyboard, then the rule-view is
> the active element, and therefore we know that ctrl+F should focus the
> rule-view search field.
> One drawback of this approach is that it adds one element that you need to
> go through when tabbing.

Yura: as Patrick said in the comment above, this patch adds a tabindex on some containers, which therefore become focusable. Which means the user has to go through those containers when tabbing in the devtools. Is this ok in your opinion?

Comment 11

[Marco Zehe (:MarcoZ)]

tab-focusing containers is actually always bad UX, since containers are non-interactive. If they need to be made focusable for some reason, it is better to use tabindex="-1" instead, which makes them focusable, but tab will skip over them.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> ::: devtools/client/inspector/test/browser_inspector_search-sidebar.js:54
> (Diff revision 2)
> > +
> > +function clickInRuleView(inspector) {
> > +  let win = inspector.panelDoc.defaultView;
> > +  let el = inspector.panelDoc.querySelector("#sidebar-panel-ruleview");
> > +  EventUtils.synthesizeMouseAtCenter(el, {type: "mousedown"}, win);
> > +  EventUtils.synthesizeMouseAtCenter(el, {type: "mouseup"}, win);
> 
> I think you can use 
> 
>   EventUtils.synthesizeMouseAtCenter(el, {}, win);
thanks, done.

(In reply to Marco Zehe (:MarcoZ) from comment #11)
> tab-focusing containers is actually always bad UX, since containers are
> non-interactive. If they need to be made focusable for some reason, it is
> better to use tabindex="-1" instead, which makes them focusable, but tab
> will skip over them.
Ah, thanks Marco. I have made that change and it seems to work great locally.

Comment 13

[Julian Descottes [:jdescottes]]

Clearing needinfo for :yzen as Marco replied already. 

Patrick, looks like tabindex=-1 would work for us here. Feel free to update your patch accordingly and carry over the r+.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8771935 [details]
Bug 1278552 - Make sidebar containers focusable so shortcuts work when they are active;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64884/diff/2-3/

Comment 15

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a764cbe3c13e&group_state=expanded

Comment 16

[Patrick Brosset <:pbro>]

I appears this causes an old regression fixed by bug 1245996 to come back again.
When an input field is focused in the rule-view, we don't want the scrollbars being dragged to blur it. But now, since the container is focusable, dragging a scrollbar does focus it and blur the input field.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset <:pbro> from comment #5)
> This is a first attempt at fixing this.
> Perhaps we should have a more generic approach on the long run. If our goal
> is to reduce even further the number of iframes, then perhaps we could have
> something at toolbox level that detects user activity and routes shortcuts
> to the right panel.

Given the difficulty / fallout that caused in this one case, I'd like to have some clear measurements that show that reducing the number of iframes is a worthwhile goal.  Granted, some of the issues from the inspector side panels change were related to XUL / HTML mixing, but things like this issue and CSS interference seem like they will only get worse if we try to somehow build multiple tools within the same frame.

I'd like to ultimately see:

toolbox-wrapper.xul
  toolbox.html
    inspector.html
    console.html
    debugger.html
    etc

Then within individual tools, it should generally be up to the tool owner for determining when subframes make sense

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #16)
> I appears this causes an old regression fixed by bug 1245996 to come back
> again.
> When an input field is focused in the rule-view, we don't want the
> scrollbars being dragged to blur it. But now, since the container is
> focusable, dragging a scrollbar does focus it and blur the input field.
So, if I set tabindex=-1 on a descendant of the container that has the scrollbar, then this issue goes away. As long as its the overflowing element itself or a parent, then focus is lost when you start dragging. So I could insert a new container just below the overflowing element that is used solely as a focusable container.

Comment 19

[Patrick Brosset <:pbro>]

Made a fix based on comment 18, here's a new try push for it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24c87c0131c&group_state=expanded

Comment 20

[Patrick Brosset <:pbro>]

I had another rebase to handle. So, new try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56286b66ec6a&group_state=expanded

Comment 21

[Patrick Brosset <:pbro>]

I won't be able to uplift this to 49 and 48. There has been substantial changes in the inspector that this patch now relies on, and so I'd have to either make a completely new custom patch, or uplift also the other bugs.
Safer just holding off.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/129c925e216d
Make sidebar containers focusable so shortcuts work when they are active; r=jdescottes

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/129c925e216d

Comment 24

[Nazir Ahmed Sabbir [:NaSb]]

I've managed this issue on this bug in Nightly 50.0a1 (2016-06-07) ; (Build ID: 20160607030217) from Linux.

This Bug is now verified as fixed on Latest Firefox Nightly 50.0a1 (2016-07-25)

Build ID: 20160725030248
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
OS: Linux 4.4.0-31-generic ; Ubuntu 16.04 (64 Bit)

Comment 25

[Mohammad Maruf Rahman]

 I have reproduced this bug with Nightly 50.0a1 (2016-06-07) on Windows 7, 64 Bit!

 This bug's fix is verified on latest Aurora 

 Build ID     20160803004014

 User Agent   Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0


 [bugday-20160803]