Fix overlapping borders in the ruleview-header

RESOLVED FIXED in Firefox 47

Status

P3
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 47

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8725432 [details]
Screenshot

We can see an overlaying border when we have a couple of situations:
(1) The pseudo-elements container is present (Enable Browser Styles)
(2) The pseudo-elements container is not expanded and the next element is a ruleview-header
(Assignee)

Updated

3 years ago
Depends on: 1250323
(Assignee)

Comment 1

3 years ago
Created attachment 8725454 [details] [diff] [review]
1252640.patch
Attachment #8725454 - Flags: review?(pbrosset)
Comment on attachment 8725454 [details] [diff] [review]
1252640.patch

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

So I've got one problem with this patch: today the display between MacOS and Windows is pretty different:

- On MacOS, when there are 2 collapsible sections next to each other, they touch, and so you end up with the double border problem that you're aiming at fixing here.
- On Windows however, the 2 sections are far apart (like maybe 3 or 4px apart), and so the borders don't touch.

Therefore, this patch makes it look a little bit weird on Windows I'd say.
I'll attach before/after screenshots.
I do think we should have the same display on both OS.

::: devtools/client/inspector/rules/rules.js
@@ +1097,3 @@
>     */
> +  _toggleContainerVisibility: function(twisty, container, isPseudo,
> +      showPseudo) {

super nit: please align showPseudo with twisty.
When we wrap long lines, we either indent the second line with 2 extra spaces, or try to find a good place to vertically align the second line. For functions, vertically aligning with the other arguments above is often a good thing:

_toggleContainerVisibility: function(twisty, container, isPseudo,
                                     showPseudo) {
Attachment #8725454 - Flags: review?(pbrosset)
(Assignee)

Comment 6

3 years ago
Created attachment 8726104 [details] [diff] [review]
1252640.patch [1.0]
Attachment #8725454 - Attachment is obsolete: true
Attachment #8726104 - Flags: review?(pbrosset)
I had to back this out because it appeared to cause this test failure: https://treeherder.mozilla.org/logviewer.html#?job_id=7758967&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/4cea7a5931c3
Flags: needinfo?(gl)
(Assignee)

Updated

3 years ago
Flags: needinfo?(gl)
That test is still failing, so I'm backing out everything from that original push:
https://hg.mozilla.org/integration/fx-team/rev/4d65c8ec069b
Flags: needinfo?(gl)
(Assignee)

Updated

3 years ago
Flags: needinfo?(gl)

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2a269c7c7bb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.