Closed
Bug 1260121
Opened 8 years ago
Closed 8 years ago
[markupview] Pseudo class lock indicator should not appear underneath tree twisty
Categories
(DevTools :: Inspector, defect, P1)
Tracking
(firefox47 unaffected, firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | fixed |
People
(Reporter: ntim, Assigned: gl)
References
(Depends on 1 open bug)
Details
(Keywords: regression, Whiteboard: [btpp-fix-now])
Attachments
(2 files, 2 obsolete files)
2.61 KB,
image/png
|
Details | |
2.45 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
I'm guessing gl's recent changes regressed this.
Reporter | ||
Updated•8 years ago
|
Summary: Pseudo class lock indicator should not appear underneath tree twisty → [markupview] Pseudo class lock indicator should not appear underneath tree twisty
Comment 1•8 years ago
|
||
Here's a screenshot of the problem. Also, some steps to reproduce: - open data:text/html,<div> - open the inspector - right-click on <div> in the markup-view and choose :hover at the bottom of the ctx menu. It looks like the padding/margin/position of various elements to the left of the markup-view changed and that caused the pseudo-class marker to overlay the twisty expander. We *really* need some automated screenshot tests to prevent these sort of regressions in the future.
Comment 2•8 years ago
|
||
Marking as a P1 because we should not ship the next release without this fixed. As Tim said, this is a regression from bug 1254786, specifically, adding position:relative to the .children rule forced the pseudo-class marker to be positioned into this container rather than into the line.
Comment 3•8 years ago
|
||
Instead of position: relative and ::before with pos: absolute, something like this might work (could surely use some optimizations and double checking on the magic numbers but it seems OK locally) .tag-line[selected] + .children { background-size: 1px 1px; background-image: linear-gradient(to top, red 1px, transparent); background-repeat: repeat-y; background-position: -6px 0px; border-left: 16px solid transparent; margin-left: -16px; }
Comment 4•8 years ago
|
||
I also imagine border-image could somehow be used for this
Assignee | ||
Comment 5•8 years ago
|
||
Comment 6•8 years ago
|
||
Comment on attachment 8736081 [details] [diff] [review] 1260121.patch Review of attachment 8736081 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/markup.css @@ +144,3 @@ > } > .tag-line:hover:not([selected]) + .children::before { > opacity: 0.5; Should this rule be removed / updated? Looks like it might be left over as part of the hover state
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8736081 -
Attachment is obsolete: true
Attachment #8736081 -
Flags: review?(pbrosset)
Attachment #8736110 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•8 years ago
|
Attachment #8736110 -
Flags: review?(bgrinstead) → review?(pbrosset)
Comment 8•8 years ago
|
||
Comment on attachment 8736110 [details] [diff] [review] 1260121.patch [1.0] Review of attachment 8736110 [details] [diff] [review]: ----------------------------------------------------------------- I'm fine with the approach, and this fixes the problem. I just would like you investigate ways to make the code easier to maintain. ::: devtools/client/themes/markup.css @@ +135,5 @@ > padding-left: 2px; > } > > +.tag-line[selected] + .children { > + background-size: 1.5px 1px; Can you explain (in a comment) why 1.5px is needed here. At least on my non-retina screen, 1.5px and 2px look exactly the same. @@ +140,5 @@ > + background-image: linear-gradient(to top, var(--markup-outline) 1px, transparent); > + background-repeat: repeat-y; > + background-position: -6px 0px; > + border-left: 16px solid transparent; > + margin-left: -16px; I'm not too sure about the 16px dimension here. And we might want to use variables to avoid these numbers here. In fact, if I replace 16px by 6px in both the border and margin, it works the same for me. And I'm guessing 6px is the twisty's width, right? So maybe this would help make it more maintainable: --twisty-width: 6px; background-position: calc(-1 * var(--twisty-width)) 0px; border-left: var(--twisty-width) solid transparent; margin-left: calc(-1 * var(--twisty-width)); I've become very fond of using vars and calc in CSS, they bring much of what people like sass/less for, and make the code easier to maintain. I'm open to discussion if you think it feels too complex. But at the very least, we should have a comment explaining why we use magic px numbers here (and again, 16px works the same as 6px for me for some reasons).
Attachment #8736110 -
Flags: review?(pbrosset) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
1.5px is the width of the outline that Helen agreed to in the previous design. I switched it to 6px to be consistent with the background position -6px. The width of the twisty arrow is 14px and we don't formally have a reason for the 6px or 16px other than the fact that it works. We could try to correlate the 6px to the the background position, but hopefully with the comments I added that will be sufficient.
Attachment #8736110 -
Attachment is obsolete: true
Attachment #8736403 -
Flags: review?(pbrosset)
Updated•8 years ago
|
Attachment #8736403 -
Flags: review?(pbrosset) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/fx-team/rev/a15b5e6efd30
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a15b5e6efd30
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 12•8 years ago
|
||
Using a simple border-left:2px solid will also work, instead of using the background-image/size/etc: .tag-line+.children{ border-left:2px solid transparent; margin-left:-5px; padding-left:3px} .tag-line:hover+.children{ border-left-color:#CCCCFF} .tag-line[selected]+.children{ border-left-color:var(--markup-outline)} (Note, I have added the hover effect, so to also display this line when hovering a group)
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Alfred Kayser from comment #12) > Using a simple border-left:2px solid will also work, instead of using the > background-image/size/etc: > .tag-line+.children{ > border-left:2px solid transparent; > margin-left:-5px; > padding-left:3px} > .tag-line:hover+.children{ > border-left-color:#CCCCFF} > .tag-line[selected]+.children{ > border-left-color:var(--markup-outline)} > > (Note, I have added the hover effect, so to also display this line when > hovering a group) Hi Alfred, Your solution would be appropriate. There were some additional reasoning behind using background-image and size. Using border would slightly push the content to the right. We were also going with a particular design where the line would be aligned directly under the expander arrow, and we wanted a 2px padding from the top and bottom so the the line is not uniformly top to bottom.
Updated•8 years ago
|
Version: unspecified → 48 Branch
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•