Closed Bug 1260121 Opened 4 years ago Closed 4 years ago

[markupview] Pseudo class lock indicator should not appear underneath tree twisty

Categories

(DevTools :: Inspector, defect, P1)

48 Branch
defect

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)

I'm guessing gl's recent changes regressed this.
Summary: Pseudo class lock indicator should not appear underneath tree twisty → [markupview] Pseudo class lock indicator should not appear underneath tree twisty
Attached image pseudo-expander.png
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.
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.
Blocks: 1254786
Keywords: regression
Priority: -- → P1
Whiteboard: [btpp-fix-now]
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;
}
I also imagine border-image could somehow be used for this
Attached patch 1260121.patch (obsolete) — Splinter Review
Assignee: nobody → gl
Status: NEW → ASSIGNED
Attachment #8736081 - Flags: review?(pbrosset)
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
Attached patch 1260121.patch [1.0] (obsolete) — Splinter Review
Attachment #8736081 - Attachment is obsolete: true
Attachment #8736081 - Flags: review?(pbrosset)
Attachment #8736110 - Flags: review?(bgrinstead)
Attachment #8736110 - Flags: review?(bgrinstead) → review?(pbrosset)
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+
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)
Attachment #8736403 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a15b5e6efd30
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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)
(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.
Version: unspecified → 48 Branch
Depends on: 1327977
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.