1224669 - mouse over menu issue at baystreet.ca, due to negative "letter-spacing" throwing off intrinsic sizing calculation

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Wenqing Jiang]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

go to baystreet.ca, mouse over the menu item (like Market), a submenu will show up.  Try to select a submenu item, and you'll find that once your cursor moves away from the main menu item, the submenu disappears before your cursor can reach any if its items.
Try Google Chrome and Apple Safari, they are all working well.  (IE 10, 11 and MS Edge don't even show the main menu so forget about them).

Comment 1

[ovidiu boca[:Ovidiu]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151116030208


Hi Wenqing,

I can reproduce your problem on Nightly 45.0a1.

Steps to reproduce:

1 Go to www.baystreet.ca
2 Mouse over the menu item like Market and a submenu will show up.
3 Try to select a submenu item

Expected results:
I can select a submenu item

Actual results:
Once your cursor moves away from the main menu item, the submenu disappears before your cursor can reach any if its items.

Comment 2

[Stefan [:stefanh]]

I doubt that this is mac-specific and I also don't think it's a widget issue. This looks more related to css parsing, in http://www.baystreet.ca/css/all.css there are rules like
#nav ul > li.hover .dropdown {
	display: block;
}

It's a bit of a guess, but "CSS Parsing and Computation" might be a better component here.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: screen shot (Chrome)

Comment 4

[Cameron McCormack (:heycam)]

Attachment: screen shot (Firefox)

Comment 5

[Cameron McCormack (:heycam)]

I added red borders around to the two relevant elements (the one that you hover over, and the popup) and you can see that in Chrome the elements overlap a little so that you can go from the "Market" to the popup without leaving the "Market" element.  (The popup is a child of the "Market" element.)  In Firefox the popup looks too low, and when you move the mouse over the gap a mouseout element triggers that hides the popup.

I haven't looked into the reason for the different layout.

Comment 6

[Cameron McCormack (:heycam)]

The links in the top right of the page, "Login  Become a Member  View Desktop Site", are on a single line in Chrome but wrap to two lines in Firefox.  That taller box causes the popup to be placed lower, resulting in the gap.  That box, the div.header-box, is float:right and so should shrink wrap to the width of its contents, and I can't see what is limiting its width here.  Daniel, is this something you could look at?

Comment 7

[Daniel Holbert [:dholbert]]

Taking a look. Clues:
 * I can only reproduce if the viewport is 1440px wide or smaller.
 * The key thing that happens at 1440px is that the "View Desktop Site" label appears at top-right.
 * In Firefox, this text takes up its own line, for some reason. In Chrome, it's on the same line as the text before it. (Not sure why yet.)
 * If I use devtools to make the text taller in Chrome (as if it were on a second line), then I can introduce the bug in Chrome.

Now looking into why that text is/isn't on the same line as the text before it.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a testcase. The text here is all on the same line in Chrome; it's on 2 different lines in Firefox.

The "letter-spacing: -6px;" style seems to be the key -- that makes us undershoot the max-content intrinsic size calculation for the floated div, and it ends up not being wide enough to fit its contents all on one line, and we end up needing to add a linebreak.

Comment 9

[Daniel Holbert [:dholbert]]

(I think this is similar to bug 761645, possibly a duplicate.)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (with larger negative letter-spacing)

It looks like, during intrinsic sizing calculations, we'll gladly honor an arbitrarily-negative "letter-spacing" value, and we'll let that make the space character here have negative width, producing a small max-content value for the floated element.

Then when we get to actually doing layout, the space character ends up with a 0 width (not a negative width), so we overflow & need to wrap.

Comment 11

[Daniel Holbert [:dholbert]]

CC'ing some text layout folks. jfkthame, any chance you've got cycles to take a look at this?

Comment 12

[Cameron McCormack (:heycam)]

Nice detective work, Daniel!

Comment 13

[Jonathan Kew [:jfkthame]]

Agree with comment 10, it looks like that's what is happening. So to fix this, we either need to make the actual layout process handle negative-width frames (like the textframe with the space in attachment 8702952 [details]), or we could clamp the used value of letter-spacing such that overall textframe width never becomes negative.

(Note that https://drafts.csswg.org/css-text-3/#letter-spacing-property says that "Values may be negative, but there may be implementation-dependent limits", so the spec allows us to limit negative values to something reasonable from an implementation POV.)

The simplest fix here, I think, is just to clamp negative results from MeasureTextWidth in nsTextFrame::AddInlinePrefISizeForFlow so that we treat such a run as having zero width for intrinsic-sizing purposes. That seems to match how it behaves during layout, and to be interoperable with Blink/Edge behavior on this testcase.

IMO, rendering behavior might be further improved by clamping negative letter-spacing between each character pair so as to prevent it ever "backspacing" within the text run. That sounds like a significantly more complex fix, however, and may be less interoperable. I'll give it a bit more thought, and perhaps file a separate bug for it.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Reftest with extreme negative letter-spacing affecting intrinsic width

Here's a reftest version of your testcase #2, currently fails on trunk.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: Clamp width to be non-negative during intrinsic width calculation

And here's a patch that seems to fix the problem (as well as bug 761645's example, incidentally). Try run in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1146d18c3ebc.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8703135 [details] [diff] [review]
Reftest with extreme negative letter-spacing affecting intrinsic width

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

::: layout/reftests/text/negative-letter-spacing-1.html
@@ +6,5 @@
> +    div {
> +      /* This letter-spacing value only impacts a single space character.
> +         By using a sufficiently-negative value, we can make this div
> +         have an arbitrarily-small max-content size (as if the space
> +         had negative width, but only for max-content sizing purposes).

Let's drop my /**/ comment here (or at least drop everything after the first line).  It will no longer be true, once this bug is fixed.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8703135 [details] [diff] [review]
Reftest with extreme negative letter-spacing affecting intrinsic width

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

::: layout/reftests/text/reftest.list
@@ +325,5 @@
>  # font fallback for <space> when not supported in the primary font family - bug 970891
>  HTTP(..) == space-font-1.html space-font-1-ref.html
> +
> +# handling of negative letter-spacing and intrinsic width, bug 1224669
> +fails == negative-letter-spacing-1.html negative-letter-spacing-1-ref.html

One other nit -- I'm not sure it makes sense to mention bug numbers in a heading in the reftest.list file like this.  Suppose we file a completely-different bug about negative letter spacing, and add "negative-letter-spacing-2.html" under negative-letter-spacing-1.html here -- it's a bit odd for that new test to be associated with this bug number.

Not a big deal, though; fine either way.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8703136 [details] [diff] [review]
Clamp width to be non-negative during intrinsic width calculation

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

Thanks for the quick fix! r=me, nits below:

::: layout/generic/nsTextFrame.cpp
@@ +7873,5 @@
>  
>      if (i > wordStart) {
>        nscoord width =
> +        NSToCoordCeilClamped(textRun->GetAdvanceWidth(wordStart, i - wordStart,
> +                             &provider));

&provider is mis-indented here -- should be below "wordStart,"

@@ +8031,5 @@
>      if (i > lineStart) {
>        nscoord width =
> +        NSToCoordCeilClamped(textRun->GetAdvanceWidth(lineStart, i - lineStart,
> +                                                      &provider));
> +      width = std::max(0, width); // ensure non-negative (bug 1224669)

Maybe drop the comment here, since
 (1) the code is pretty self-explanatory.
 (2) the 3 other max() invocations that you're adding in this patch lack any such comment.
 (3) When "bug NNN" is dropped into a code-comment (without context), it often implies "something is currently broken here, and it'll be fixed in bug NNN", and that's not the case here. (And IIRC, Jesse or someone occasionally scans the codebase for mentions of "bug NNN" where NNN is a fixed bug, to see if any such comments have been accidentally left in - and comments like this would create noise for that process.) (and hg blame provides the same information RE the bug number that you're trying to provide here.)

Comment 19

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68fbc021bdb4b84b84dbf3d7899cde7d165bc07a
Bug 1224669 - Reftest with extreme negative letter-spacing affecting intrinsic width. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8d40c30f5bdc6187d60300b6a3cab4728cd043
Bug 1224669 - Clamp width to be non-negative during intrinsic width calculation. r=dholbert

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68fbc021bdb4
https://hg.mozilla.org/mozilla-central/rev/8d8d40c30f5b