>>> 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

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.

Inspector bug triage (filter on CLIMBING SHOES).

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

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

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.

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

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 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 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

(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?

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.

(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.

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 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/

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.

(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

(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.

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

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

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.

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

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)

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]