Closed Bug 1271899 Opened 8 years ago Closed 8 years ago

Clicking outside of text fields in box model doesn't collapse them

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Version:
47 Branch
Type:
defect

Tracking

(firefox47 wontfix, firefox48 verified, firefox49 verified, firefox50 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox47 --- wontfix
firefox48 --- verified
firefox49 --- verified
firefox50 --- verified

People

(Reporter: arni2033, Assigned: jdescottes)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

MozReview Request: Bug 1271899 - add div wrapper in inspector layout panel;r=gl
58 bytes, text/x-review-board-request
gl
: review+
gchang
: approval-mozilla-aurora+
gchang
: approval-mozilla-beta+
Details
bug1271899.beta.patch
16.91 KB, patch
Details | Diff | Splinter Review
bug1271899.eslint-fix.patch
2.36 KB, patch
Details | Diff | Splinter Review 
>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160504043118
STR:
1. Open url   data:text/html,<body></body>
2. Open inspector -> box model
3. Click on margin-bottom value
4. Click below all colored rectangles in "box model" tab

AR:  Text field opened for editing margin-bottom is still open and focused
ER:  Text field should close

This is regression from bug 1245996. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=55991a2134a089f328988760ed79601dad8a4cd6&tochange=cc1448afa617045d5b00c2ed183ec5b5707ee1f8
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
The XUL tabpanel can no longer receive userfocus to avoid scrollbars
stealing the focus. Editors are using the blur event to destroy
themselves, which was no longer working if a user click ended up on
the tabpanel element.

Add a wrapper inside the layout view panel taking
100% width and height to make sure clicks performed outside of the layout
container will trigger a blur event on a focused editor.

Review commit: https://reviewboard.mozilla.org/r/56310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56310/
Attachment #8757969 - Flags: review?(gl)
Attachment #8757969 - Flags: review?(gl) → review+
Comment on attachment 8757969 [details]
MozReview Request: Bug 1271899 - add div wrapper in inspector layout panel;r=gl

https://reviewboard.mozilla.org/r/56310/#review54684
Thanks for the review Gabriel!
Green try except for unrelated intermittents. Landing.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/b6d9770a62ab
inspector box-model: add div wrapper to close editors on click;r=gl
https://hg.mozilla.org/mozilla-central/rev/b6d9770a62ab
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Should we consider this for uplift?
status-firefox47: --- → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected
Flags: needinfo?(jdescottes)
Version: unspecified → 47 Branch
An uplift would be nice here, users unable to "close" the inputs in the box-model panel might think the UI is broken.
Flags: needinfo?(jdescottes)
Comment on attachment 8757969 [details]
MozReview Request: Bug 1271899 - add div wrapper in inspector layout panel;r=gl

Approval Request Comment
[Feature/regressing bug #]: regression from Bug 1245996
[User impact if declined]: devtools users unable to close inplace inputs in the box-model view. Feels like the UI is buggy, not responsive, gives a bad impression.
[Describe test coverage new/current, TreeHerder]: New test added in this patch (https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=1828937da949)
[Risks and why]: low risk, small markup & css change
[String/UUID change made/needed]: none
Attachment #8757969 - Flags: approval-mozilla-aurora?
Comment on attachment 8757969 [details]
MozReview Request: Bug 1271899 - add div wrapper in inspector layout panel;r=gl

Approval Request Comment
[Feature/regressing bug #]: regression from Bug 1245996
[User impact if declined]: devtools users unable to close inplace inputs in the box-model view. Feels like the UI is buggy, not responsive, gives a bad impression.
[Describe test coverage new/current, TreeHerder]: New test added in this patch (https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=1828937da949)
[Risks and why]: low risk, small markup & css change
[String/UUID change made/needed]: none
Attachment #8757969 - Flags: approval-mozilla-beta?
Comment on attachment 8757969 [details]
MozReview Request: Bug 1271899 - add div wrapper in inspector layout panel;r=gl

This patch fixes a regression. Take it in 48 beta 8 and aurora.
Attachment #8757969 - Flags: approval-mozilla-beta?
Attachment #8757969 - Flags: approval-mozilla-beta+
Attachment #8757969 - Flags: approval-mozilla-aurora?
Attachment #8757969 - Flags: approval-mozilla-aurora+
QA Whiteboard: [good first verify]
The test browser_layout_editablemodel_bluronclick.js was relying on a util method that was added in another bug [1].

In the patch attached here I added the missing method and folded it in my previous patch. I assume uplifted patches are never merged back to mozilla-central so this should not be an issue to do this here? Let me know if I should proceed differently.

[1] https://hg.mozilla.org/mozilla-central/diff/ea89f71fc38e/devtools/client/framework/test/shared-head.js#l428
Flags: needinfo?(jdescottes) → needinfo?(wkocher)
Attached patch bug1271899.beta.v2.patch (obsolete) — — Splinter Review 
I missed the ESlint failures in my previous patch, they are fixed in this new version.
Attachment #8770863 - Attachment is obsolete: true
Attachment #8770921 - Attachment is obsolete: true
Attachment #8770863 - Attachment is obsolete: false
Follow up to bug1271899.beta.patch, fixing the remaining eslint errors.

Tomcat: could you push the follow up on mozilla-beta? thanks!
Flags: needinfo?(cbook)
(In reply to Julian Descottes [:jdescottes] from comment #21)
> Created attachment 8770923 [details] [diff] [review]
> bug1271899.eslint-fix.patch
> 
> Follow up to bug1271899.beta.patch, fixing the remaining eslint errors.
> 
> Tomcat: could you push the follow up on mozilla-beta? thanks!

landed as https://hg.mozilla.org/releases/mozilla-beta/rev/306382512096
Flags: needinfo?(cbook)
I have reproduced this bug with Nightly 49.0a1 (2016-05-11) on Ubuntu 16.04, 64 bit!
The bug's fix is now verified on latest Nightly 50.0a1, Aurora 49.0a2, Beta 48.0b9.

Nightly 50.0a1:
Build ID 	20160722030235
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

Aurora 49.0a2:
Build ID 	20160722004032
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0

Beta 48.0b9:
Build ID 	20160718142219
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0

[testday-20160722]
I have reproduced this bug with Firefox Nightly 49.0a1 (Build ID:20160511030221) on 
Windows 8.1, 64-bit.

Verified as fixed with Firefox beta 48.0b9 (Build ID: 20160718142219)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

Verified as fixed with Firefox Developer edition 49.0a2 (Build ID: 20160723004004)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

Verified as fixed with Firefox Nightly 50.0a1 (Build ID: 20160722030235)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

As this bug is also verified on Ubuntu(comment 23), I am marking this as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][testday-20160722]
status-firefox48: fixed → verified
status-firefox49: fixed → verified
status-firefox50: fixed → verified
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: