Closed Bug 1252640 Opened 4 years ago Closed 4 years ago

Fix overlapping borders in the ruleview-header


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



(firefox47 fixed)

Firefox 47
Tracking Status
firefox47 --- fixed


(Reporter: gl, Assigned: gl)




(4 files, 1 obsolete file)

Attached image 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
Depends on: 1250323
Attached patch 1252640.patch (obsolete) — Splinter Review
Attachment #8725454 - Flags: review?(pbrosset)
Comment on attachment 8725454 [details] [diff] [review]

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)
Attachment #8725454 - Attachment is obsolete: true
Attachment #8726104 - Flags: review?(pbrosset)
Attachment #8726104 - Flags: review?(pbrosset) → review+
Flags: needinfo?(gl)
That test is still failing, so I'm backing out everything from that original push:
Flags: needinfo?(gl)
Flags: needinfo?(gl)
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.