Closed Bug 1245996 Opened 8 years ago Closed 8 years ago

Textareas in ruleview collapse when I click on the vertical scrollbar

Categories

(DevTools :: Inspector: Rules, defect)

Product:
DevTools
Component:
Inspector: Rules
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: arni2033, Assigned: jdescottes)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

MozReview Request: Bug 1245996 - inspector: fix xul scrollbars stealing focus;r=pbro
58 bytes, text/x-review-board-request
pbro
: review+
Details 
>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160204030229
STR:
1. Open New Tab Page
2. Open devtools, dock it to the bottom side of window. Open Inspector tab, select <body> in markup
3. Resize devtools vertically so that scrollbar appeared in "Rules" tab in sidebar
4. Click on any existing rule in ruleview
5. Click on scrollbar in "Rules" tab

AR: Textarea collapsed
ER: Textarea shouldn't collapse, it should still be foxused

This is minor regression (by bug 1241126 or bug 1214177). Here's the best regression range I got:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d07dbd40dcd209124149f331f60dd55c8da33fbe&tochange=eb673021691c3c3257546995edf4731a1a7c165c
s/foxused/focused

Julian, can you determine what caused this w/o using mozregression?
Flags: needinfo?(jdescottes)
It is linked to Bug 1238133 and Bug 1214177. Part 2 of Bug 1214177 fixed a regression of Bug 1238133 by re-enabling the focus on inspector tabpanel elements. 

From some quick testing, we should only enable user-focus html:div elements inside the tabpanel, and not on the tabpanel itself. Otherwise, scrollbars can steal the focus from inputs, which creates the issue you reported.

FWIW, this does not occur on OSX.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
Has STR: --- → yes
Attached patch bug1245996.wip.patch (obsolete) — — Splinter Review 
Bug 1245996 - inspector: fix xul scrollbars stealing focus

Simply adding "> *" to the selector of the rule granting focus to tabpanel elements. This fixes the issue locally for me.

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3ae76c44a39
Attached patch bug1245996.v1.patch (obsolete) — — Splinter Review 
(waiting for try before flagging for r?)
try https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdbb1449e241

Bug 1245996 - inspector: fix xul scrollbars stealing focus;r=pbro

Apply -moz-user-focus: normal; only on direct descendants of inspector
tabpanels and not on tabpanels themselves. Avoids XUL scrollbars
from stealing the focus when clicked.
Attachment #8716231 - Attachment is obsolete: true
Attached patch bug1245996.v2.patch (obsolete) — — Splinter Review 
:pbro : not convinced about the test here. The one attached in the patch works, except on OSX, where it is skipped. Would be tempted to only commit the code fix here, let me know what you think.

In the meantime : new try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb0f5fdfb11 .

Bug 1245996 - inspector: fix xul scrollbars stealing focus;r=pbro

Apply -moz-user-focus: normal; only on direct descendants of inspector
tabpanels and not on tabpanels themselves. Avoids XUL scrollbars
from stealing the focus when clicked.

Test added, run only on windows and linux
Attachment #8716244 - Attachment is obsolete: true
Attachment #8716646 - Flags: feedback?(pbrosset)
Given the recent developments in Bug 1243598, it looks like the changes from Bug 1238133 might be reverted. This would also solve this bug.

Let's wait until 1243598 is resolved before committing anything here.
Depends on: 1243598
Comment on attachment 8716646 [details] [diff] [review]
bug1245996.v2.patch

Review of attachment 8716646 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have a particular problem with this test, apart from the fact that it doesn't run on OSX. But a test on other platforms is better than no tests at all. Isn't there a pref we can set to force non-floating scrollbars on OSX? Floating scrollbars are only just a CSS we apply I think. Brian would know better.
(As you mentioned in comment 6, this all of course depends if we rollback bug 1238133).

::: devtools/client/inspector/rules/test/browser_rules_edit-selector-click-on-scrollbar.js
@@ +42,5 @@
> +  yield selectNode(".testclass", inspector);
> +
> +  let container = view.element;
> +  let hasScrollbar = container.offsetHeight < container.scrollHeight;
> +  ok(hasScrollbar, "The rule view container should have a vertical scrollbar.");

This is unlikely, but if the toolbox is suddenly a lot taller than it is now by default (for instance if a previous test in the suite makes it taller and forgets to reset the default height), then we might not have a scrollbar.
I think this is controlled by the pref devtools.toolbox.footer.height. A suggestion would be for your test to set that pref to something sensible when it starts and then clear it when it ends.

@@ +59,5 @@
> +  yield onScroll;
> +  info("The rule view container scrolled after clicking on the scrollbar.");
> +
> +  is(editor.input, view.styleDocument.activeElement,
> +    "The editor input should still be focused.");

I've taken the habit of blurring input fields before tests end. The reason is that we do things when they blur like, sometimes, call the server. So if you don't blur the field yourself, it's going to be blurred when the test ends, and that will trigger the corresponding server calls (or whatever we do in that case) and potentially cause unhandled promise rejections as the server would have been destroyed in the meantime.
In this specific case however, it looks like nothing happens on blur of a selector that hasn't changed. So we're fine.
Attachment #8716646 - Flags: feedback?(pbrosset) → feedback+
Attachment #8716646 - Attachment is obsolete: true
Apply -moz-user-focus: normal; only on direct descendants of inspector
tabpanels and not on tabpanels themselves. Avoids XUL scrollbars
from stealing the focus when clicked.

Test added, run only on windows and linux

Review commit: https://reviewboard.mozilla.org/r/35449/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35449/
Attachment #8720749 - Flags: review?(pbrosset)
Comment on attachment 8720749 [details]
MozReview Request: Bug 1245996 - inspector: fix xul scrollbars stealing focus;r=pbro

https://reviewboard.mozilla.org/r/35449/#review32127

Ship it!
Attachment #8720749 - Flags: review?(pbrosset) → review+
Thanks for the review.
Try is green.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08731b65f242
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1271899
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: