Closed Bug 1139925 Opened 9 years ago Closed 9 years ago

Devtools highlighter shows incorrect sizes for elements which correspond to multiple CSS boxes.

Categories

(DevTools :: Inspector, defect)

39 Branch
x86_64
Linux
defect
Not set
normal

Tracking

(firefox36 unaffected, firefox37+ wontfix, firefox38+ verified, firefox39+ verified, firefox-esr31 unaffected)

VERIFIED FIXED
Firefox 39
Tracking Status
firefox36 --- unaffected
firefox37 + wontfix
firefox38 + verified
firefox39 + verified
firefox-esr31 --- unaffected

People

(Reporter: gmarty, Assigned: pbro)

References

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

Open http://jsbin.com/vitifeveve/1 in nightly.
It is a RTL document containing a <label> element with a ::before pseudo element.

The label width is 0. It creates all sort of issue when styling the pseudo ::before element.

Also the width as reported by the inline debug and the Box Model tab of the dev tools differ (0 vs. 204px).

It is a regression as both beta and aurora are fine. Firefox OS v3 is also affected.
> The label width is 0.

I tried loading http://jsbin.com/vitifeveve/1 and doing the following in the console:

1)  document.getElementById("suggestions-provider").offsetWidth -- returns 181
2)  document.getElementById("suggestions-provider").getBoundingClientRect().width --
    returns 180.8166etc
3)  document.getElementById("suggestions-provider").style.background = "yellow" -- gives
    a yellow background to all the text.

Those are all correct.  So the only issue here is with scrollWidth, right?

Are you willing to narrow down the regression range, since this is a regression?
Flags: needinfo?(gmarty)
Summary: Label with ::before element has a 0 width in RTL → Label with ::before element containing only spaces has a 0 scrollWidth in RTL
Actually, this is not specific to ::before, unsurprisingly.  Just having a first child element that contains only spaces, or is empty, is enough:

<!DOCTYPE html>
<html dir="rtl">
  <span><span></span>Some text</span>
  <pre><script>
      document.writeln(document.querySelector("span").offsetWidth);
      document.writeln(document.querySelector("span").scrollWidth);
    </script>
</html>

I should note that in Chrome the outer <span> has a 0 scrollWidth even if I remove the inner <span>, and even if the document is ltr...
Summary: Label with ::before element containing only spaces has a 0 scrollWidth in RTL → Inline whose first child contains no non-space characters has a 0 scrollWidth in RTL
I'm afraid I can't really track down the regression (Isn't it something that QA can do?).

But yes, the offsetWidth/width thing is misleading, the real issue is related to layout. I'm not sure why 2 different widths are reported by the dev tools, see attachment above.
Flags: needinfo?(gmarty)
So in terms of what goes wrong...  the frametree dump for the primary frame of the outer <span> looks like this:

Inline(span)(0)@12b5a6170 next=12b5a6e00 next-continuation=12b5a6e00 {85440,96,0,960} [state=0000100000600200] [content=133650500] [sc=12b5a5710]<
  Inline(span)(0)@12b5a61e8 {0,0,0,960} [state=0000100000e00200] [content=1336505a0] [sc=12b5a5c88]<>
>

The overall frame dump for the page, or at least the parts of it that matter, looks like this:

              Inline(span)(0)@12b5a6170 next=12b5a6e00 next-continuation=12b5a6e00 {85440,96,0,960} [state=0000100000600200] [content=133650500] [sc=12b5a5710]<
                Inline(span)(0)@12b5a61e8 {0,0,0,960} [state=0000100000e00200] [content=1336505a0] [sc=12b5a5c88]<>
              >
              Inline(span)(0)@12b5a6e00 next=12b5a6320 prev-continuation=12b5a6170 {81577,96,3863,960} vis-overflow=-30,0,3925,960 scr-overflow=0,0,3863,960 [state=0002100000a00200] [content=133650500] [sc=12b5a5710]<
                Text(1)"Some text"@12b5a6260 {0,0,3863,960} vis-overflow=-30,0,3925,960 [state=0001000090620000] [content=133650640] [sc=12b5a5d38:-moz-non-element] [run=0][0,9,T] 
              >

What scrollWidth does is use the GetPaddingRectRelativeToSelf() of the primary frame, so it doesn't work so well any time there are continuations, as here.  That's been the case since bug 833542, so Firefox 21 (though it does look to me like it considered continuations before that).

So presumably what changed is the structure of the frame tree here.
> I'm afraid I can't really track down the regression

May I ask why not?  I mean just on Firefox nightlies, e.g. using http://mozilla.github.io/mozregression/

> Isn't it something that QA can do?

In my experience, they're too overworked to do it...

> the real issue is related to layout.

Can you describe what the layout issue is, exactly?  Because I'm not seeing any in the testcase you attached.  So far the only issue I'm seeing is with the scrollWidth getter.
roc, mats, did we mean to change scrollWidth to not consider continuations in bug 833542?
Flags: needinfo?(roc)
(In reply to Not doing reviews right now from comment #5)
> > I'm afraid I can't really track down the regression
> 
> May I ask why not?  I mean just on Firefox nightlies, e.g. using
> http://mozilla.github.io/mozregression/

I thought finding a regression required me to do build, but this tool is very convenient!
So this is what I got:
22:12.64 LOG: MainThread Bisector INFO Last good revision: d101d9574541
22:12.64 LOG: MainThread Bisector INFO First bad revision: 8a0c83fe66d1
22:12.64 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d101d9574541&tochange=8a0c83fe66d1

> Can you describe what the layout issue is, exactly?  Because I'm not seeing
> any in the testcase you attached.  So far the only issue I'm seeing is with
> the scrollWidth getter.

Sorry, it's probably a rendering issue, not a layout issue.
Open the dev tool, hover the label element and see that the inline debug reports a width of 0. Have a look at the attachment above, you can see something like "label#suggestisons-provider | 0 x 19". It should really be "204 x 19".
That causes issues where I can't style the ::before pseudo element to appear on its parent left handside because it has a width of 0.

I hope it's clear enough. Let me know otherwise.
[Tracking Requested - why for this release]: Regression in devtools highlighter box size display.

> So this is what I got:

Thank you for doing the bisect!  It looks like you were doing that on the devtools behavior, not on what scrollWidth returns, right?  (And in fact, I just checked and the scrollWidth is 0 in Firefox 36, which means that there is probably no change to the structure of the frame tree recently.)

Presumably the devtools behavior change is caused by the changes from bug 1098435 which switched from getBoundingClientRect() to this.currentQuads.border.bounds. currentQuads comes from getAdjustedQuads, which does this:

44     let [quads] = node.getBoxQuads({
45       box: region
46     });

which of course ignores all but the first quad (which looks very wrong to me given the later uses of that function).

Let's focus this bug on the devtools regression.  I filed bug 1140412 on the scrollWidth bit.

> That causes issues where I can't style the ::before pseudo element to appear on its
> parent left handside

These is the part I'd like to understand.  What does one of those look like?  As in, what is the in-page layout/rendering problem you're running into, as opposed to a problem with the numbers devtools show?
Blocks: 1098435
Component: Layout → Developer Tools: Inspector
Flags: needinfo?(roc) → needinfo?(pbrosset)
Keywords: regression
Product: Core → Firefox
Summary: Inline whose first child contains no non-space characters has a 0 scrollWidth in RTL → Devtools highlighter shows incorrect sizes for elements which correspond to multiple CSS boxes.
(In reply to Not doing reviews right now from comment #8)
> Let's focus this bug on the devtools regression.  I filed bug 1140412 on the
> scrollWidth bit.

Given that this bug will focus on devtools and the regression isn't visible on Beta or Aurora (as per comment 0), I think we only need to track for 39. Please renom if I've misunderstood.
> Given that this bug will focus on devtools and the regression isn't visible on Beta or
> Aurora (as per comment 0)

I see the broken devtools behavior on beta and aurora, as expected given the regressing bug; I verified that before requesting tracking, of course.

I still don't know what exact problem comment 0 is talking about that's new in nightly; see comment 8 last paragraph.
Flags: needinfo?(gmarty)
Thanks for renoming. Tracking 37+
(In reply to Not doing reviews right now from comment #8)
> Presumably the devtools behavior change is caused by the changes from bug
> 1098435 which switched from getBoundingClientRect() to
> this.currentQuads.border.bounds. currentQuads comes from getAdjustedQuads,
> which does this:
> 
> 44     let [quads] = node.getBoxQuads({
> 45       box: region
> 46     });
> 
> which of course ignores all but the first quad (which looks very wrong to me
> given the later uses of that function).
Yes this is very wrong indeed. Also the devtools highlighter doesn't highlight inline elements that span across several lines correctly because of this.
However I don't know what's the logic behind getBoxQuads and rtl. Using the provided jsbin, executing |labelElement.getBoxQuads()| I see 2 quads return when rtl is set, and only 1 with ltr. 

Also, the regression wasn't entirely caused by bug 1098435 (which only made use of getBoxQuads to display the node's dimension in the tooltip next to the highlighter), but mostly by bug 969306 (which made the highlighter use getBoxQuads altogether, after it landed in nightly).
Flags: needinfo?(pbrosset)
I've been talking to jfkthame on irc#layout, and here's what he said, based on the minimal test case in comment 2:

"
the empty <span> is being placed before the span containing "Some text", which in an RTL doc means to its *right*.
Then because the text in the outer span is actually LTR text, we get a direction-run boundary and the actual text is rendered left-to-right, to the left of the (empty) inner span.
So even though one of them is empty, we'll have split the text here across two frames
"

I'm thinking devtools could highlight a rectangle that contains all of the returned quads instead of just using the first one. How would the highlighter know which quad to use otherwise?
This would be wrong for inline elements that span line breaks, but we're already doing it wrong today by only highlighting the first line, and anyway, we will create a new specialized highlighter for inline elements (see bug 1023232).
I'd like miker's point of view on this, but I like the outer-bounds idea.
Flags: needinfo?(mratcliffe)
Attached image Rendering difference
This is a screenshot of the issue I initially bumped into. But now I'm not sure if it's completely related to the described here.
Look at the attachment. The rendering is different between Nightly (left) and Chrome (right).
Positioning a ::before element on the left doesn't work in RTL on Firefox where it is still placed on the right of its parent. Positioning it on the right works as expected though.
But this issue also appears on Beta.

Let me know if I should open a new bug to track it.
Flags: needinfo?(gmarty)
Guillaume, thank you for that screenshot.  What you're seeing there is basically bug 489100, I suspect: We only use the first box of the label to determine the containing block for the absolutely positioned child, and in this case the first box is to the right of the text, as explained in comment 13...  It's worth filing a separate bug about this case and marking it dependent on bug 489100; it's possible that this is fixable for the bidi case without fixing it for the general line-wrapping case...
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #13)
> I've been talking to jfkthame on irc#layout, and here's what he said, based
> on the minimal test case in comment 2:
> 
> "
> the empty <span> is being placed before the span containing "Some text",
> which in an RTL doc means to its *right*.
> Then because the text in the outer span is actually LTR text, we get a
> direction-run boundary and the actual text is rendered left-to-right, to the
> left of the (empty) inner span.
> So even though one of them is empty, we'll have split the text here across
> two frames
> "
> 
> I'm thinking devtools could highlight a rectangle that contains all of the
> returned quads instead of just using the first one. How would the
> highlighter know which quad to use otherwise?
> This would be wrong for inline elements that span line breaks, but we're
> already doing it wrong today by only highlighting the first line, and
> anyway, we will create a new specialized highlighter for inline elements
> (see bug 1023232).
> I'd like miker's point of view on this, but I like the outer-bounds idea.

The outer bounds makes sense as long as the box model is actually the CSS box model and bonus points if we can highlight inline items correctly (harder than you would think).
Joe - Can you help find an owner so that we have a chance to avoid shipping this regression in 37?
Flags: needinfo?(jwalker)
I'll start looking into this in a short while.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Flags: needinfo?(mratcliffe)
Flags: needinfo?(jwalker)
/r/5347 - Bug 1139925 - Make the BoxModelHighlighter highlight all quads and draw guides around the outer-most rect

Pull down this commit:

hg pull review -r 78a7890ac547a95561ac917c1721be6ed32c5426
Attachment #8577251 - Flags: review?(mratcliffe)
Some info about this commit for Mike:

- LayoutHelpers.getAdjustedBoxQuads now returns all quads that el.getBoxQuads returns.
- The BoxModelHighlighter calculates an outer rect based on these to draw the guides.
- And if the element has more than 1 quad (inline element that spans line breaks), then all quads are
highlighted.
- Also all related tests were modified and a couple of new tests were added.

The commit looks bigger than it really is, that's mostly because I had to touch quite a lot of tests and test helper functions.

The attached screenshot shows an example of highlighting an inline element.
Comment on attachment 8577251 [details]
MozReview Request: bz://1139925/pbrosset

https://reviewboard.mozilla.org/r/5345/#review4359

Ship It!
Attachment #8577251 - Flags: review?(mratcliffe) → review+
https://hg.mozilla.org/mozilla-central/rev/a83f1ca8b751
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Patrick, looks like a few tests are still left to run on treeherder but they are mostly good. Is this something you want to uplift so that we don't ship 37 with this regression?   

It sounds like some extra QA attention to verify the fix may also be useful.
Flags: qe-verify+
Flags: needinfo?(pbrosset)
(In reply to Liz Henry (:lizzard) from comment #25)
> Patrick, looks like a few tests are still left to run on treeherder but they
> are mostly good. Is this something you want to uplift so that we don't ship
> 37 with this regression?   
In fact, this isn't a recent regression, it came with bug 969306 (target Firefox 32).
So, using Firefox release right now, I can see the same problem when trying to highlight the <label> in the example given in comment 0.
My guess on why we didn't fix this earlier is that it's not a very very common case and we just didn't see it before now.
> It sounds like some extra QA attention to verify the fix may also be useful.
The fix for this bug actually also introduces a new devtools feature: the ability to highlight all line boxes in inline elements that span line breaks. Before this, we were only highlighting the first line box.
It's also a nice feature because other browsers don't have it.
But, as with any new things, there may be hiccups.
Flags: needinfo?(pbrosset)
Given that this bug is much older than we had thought and that it is not a very very common case, I think we should ship the bug again in 37 and look to fix this in 38 or 39. We simply don't have time to deal with any fallout at this point. I have marked this bug as wontfix for 37.
> In fact, this isn't a recent regression, it came with bug 969306 (target Firefox 32).

That's not correct.  The regression came from bug 1098435, and is new in Firefox 37.  That's clearly stated in comment 0 and comment 8 and Firefox 36 is in fact not affected.  I'm not sure what Patrick is seeing in comment 26, since my Firefox 36 clearly shows a nonzero width for the label in the comment 0 testcase.

We may still want to wontfix for 37, but I want us to be clear that we _will_ be shipping a regression from 36 as a result.
Flags: needinfo?(lmandel)
Thank you for the correction bz. At this point in the schedule I think the right move is to ship with this regression. We can get this fix in to Dev Edition and onto Beta 38 soon. I'm leaving 37 as wontfix.
Flags: needinfo?(lmandel)
Patrick, can we have an uplift to beta if you think it is low risk? Thanks
Flags: needinfo?(pbrosset)
Attached image firefox34.png
(In reply to Not doing reviews right now from comment #28)
> > In fact, this isn't a recent regression, it came with bug 969306 (target Firefox 32).
> 
> That's not correct.  The regression came from bug 1098435, and is new in
> Firefox 37.  That's clearly stated in comment 0 and comment 8 and Firefox 36
> is in fact not affected.  I'm not sure what Patrick is seeing in comment 26,
> since my Firefox 36 clearly shows a nonzero width for the label in the
> comment 0 testcase.
I think I understand the confusion now. I was talking about the area described by the highlighter, and you were talking the information tooltip above the highlighter, right?
The highlighter (the overlay, with the 4 dashed lines, that appears on top of the <label> element when hovering over it in the devtools inspector) has an incorrect size since bug 969306 as I said in comment 26 (I just tested again in FF36 and FF34, and they both show the problem).
However, the information tooltip (the dark tooltip thing that appears on top, with the element tagname and dimension) did indeed regress with bug 1098435 as you mentioned in comment 28 (and earlier).

The attached screenshot (from FF34) should make that clear. We only see 3 dashed lines because the highlighter found the element to have a 0 width, and so the 2 vertical lines have the same X coordinate.
Flags: needinfo?(pbrosset)
Comment on attachment 8577251 [details]
MozReview Request: bz://1139925/pbrosset

Approval Request Comment
[Feature/regressing bug #]: Bug 1098435 introduced this regression.
[User impact if declined]: In some RTL cases (such as the one in comment 0), when using devtools, users will have the highlighter information tooltip show inline elements as having a width of 0 pixels.
[Describe test coverage new/current, TreeHerder]: Many mochitests are testing the highlighter already, and I've added a few to specifically test inline and rtl cases.
[Risks and why]: This has been in m-c (and now in aurora) for about 3 weeks, without any reports of regressions/failures, so the risk is low.
[String/UUID change made/needed]: None
Attachment #8577251 - Flags: approval-mozilla-beta?
Comment on attachment 8577251 [details]
MozReview Request: bz://1139925/pbrosset

Thanks. Should be in 38 beta 3.
Attachment #8577251 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for Beta uplift.
Flags: needinfo?(pbrosset)
This was rebased for beta.
Flags: needinfo?(pbrosset)
Reproduced this issue on Firefox 37 beta 1.

Confirming the fix on:
- Firefox 38 beta 6, build ID: 20150420134330;
- latest Aurora, build ID: 20150422004009.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
Attachment #8577251 - Attachment is obsolete: true
Attachment #8619670 - Flags: review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: