944200 - [css-ui] CSS text-overflow ellipsis overlaps float elements

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Joseph Huckaby]

Attachment: Screenshot 2013-11-27 17.26.53.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

1. Go here: http://bowser.effectgames.com/~jhuckaby/bugs/firefox-text-overflow.html
2. Notice how main text overlaps float:right block.

Explanation: The page has a simple container DIV with a set width and some text, along with a float:right block with a fixed size.  The main text is set to text-overflow:ellipsis and forced to a single line only.  However, in Firefox, the main text overlaps the float:right block.  It should wrap and have an ellipsis inserted before overlapping (as it does in Chrome and Safari).


Actual results:

Main text overlaps float:right block (see URL / screenshot).

http://bowser.effectgames.com/~jhuckaby/bugs/firefox-text-overflow.html

https://www.dropbox.com/s/0plw11wuug1sq56/Screenshot%202013-11-27%2017.26.53.png



Expected results:

Text should be cropped and have ellipsis inserted before hitting float:right block.  This is how all other browsers render my page (I tried both Chrome and Safari).

Comment 1

[:Gijs (he/him)]

I can reproduce this on latest Nightlies, so moving this to Core :: Layout.

Joseph, would you mind adding your testcase as an attachment to this bug so we'll still have it if your website or that page goes away at some point in the future? Thanks for the report! :-)

Comment 2

[Mats Palmgren (inactive)]

Firefox, IE10 and Opera(Presto) does this correctly per spec.  The overflow occurs
at the block edge and that's the edge that is relevant for ellipsing.
Please report the bug to webkit.

That said, I do sympathize with your use case.  I think it would be a useful feature
to add to the spec, something like "text-overflow-ellipsing-edge: block | line".
Feel free to propose it on [e-mail] if you want.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why would anyone ever prefer the spec behavior over the WebKit behavior?  Maybe the spec should just change, which would be far better than having an additional property.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I agree with David.  Why dies the spec say what it does?

Comment 5

[Joseph Huckaby]

Attachment: firefox-text-overflow.html

As requested, the test case as an HTML attachment, in case my server goes down.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Consider the following testcase.  In the block at the top the text
overlaps the float but does *not* overflow the block.  Still, webkit
does ellipsing here.  The middle block has the float removed, this is
just so that you can verify that the text does not overflow.  The block
at the bottom is the same as the first, except with overflow:visible.

If we change the spec to ellipsize at the line box edge instead of the
block's content edge then I think it would be logical to also ellipsize
for overflow:visible (the bottom block), because text-overflow is then
unrelated to block overflow.

Comment 7

[Kang-Hao (Kenny) Lu [:kennyluck]]

This seems like a dup of bug 864759?

Comment 8

[Joseph Huckaby]

There is a very interesting discussion about this very bug over at WebKit: https://bugs.webkit.org/show_bug.cgi?id=115746

They concluded that the text should indeed clip at the edge of the float:right DIV, not at the boundary of the outer DIV, and closed the bug.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, they did the usual webkit thing of silently deciding to ignore the spec if they don't like it without bothering to submit feedback about the spec needing to change.  :(

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Yeah, they did the usual webkit thing of silently deciding to ignore the
> spec if they don't like it without bothering to submit feedback about the
> spec needing to change.  :(

To be fair, in this case the spec is probably newer than their implementation, no?

Comment 11

[Boris Zbarsky [:bzbarsky]]

If you read the bug cited in comment 8, they checked in a patch to follow the spec as written around end of May 2013, then backed it out about a month later because they decided they wanted to keep the old behavior.

All of that very much postdates the spec.  So I think comment 9 is in fact an eminently fair summary of what they did.

Comment 12

[Mats Palmgren (inactive)]

So should we bring this to the CSSWG's attention perhaps?

Comment 13

[Mats Palmgren (inactive)]

FWIW, I don't feel strongly either way about the technical issue at hand.
I can easily change our implementation to use line box edges instead of
block edges if the CSSWG decide to change the spec.

I *do* feel strongly about Apple blatantly disregarding CSS specs though.
And when it is pointed out to them they don't even honor you with a response.
I find such behavior uncivilized, to put it mildly.

Comment 14

[Tantek Çelik]

Agreed with bz and dbaron. I also agree with Mat's additional point in Comment 6 regarding attachment 8340052 [details].

Since both require changes in several shipping implementations, yes, as Mat suggests, I'll mail www-style about it with a proposed edit to text-overflow (with an objection time-out of say a week, and then make the spec edit next week assuming no objections).

fantasai - any comment before I move forward on this?

Comment 15

[Mats Palmgren (inactive)]

This proposal was posted to www-style in Feb 2014 (by Tantek):
http://lists.w3.org/Archives/Public/www-style/2014Feb/0140.html

Tab responded "Sounds reasonable to me".  There were no other responses.

I'm not sure if the spec has been updated, the opening sentence is still:
"This property specifies rendering when inline content overflows its block container element"
http://dev.w3.org/csswg/css-ui/#text-overflow
for example, which seems wrong given the proposal.

Tantek, can I assume the proposal was accepted in the CSSWG?
When will the spec be updated?

BTW, is the revision history for this spec publicly accessible somewhere?
It would be very useful to be able to see which edits have been made.

Comment 16

[Tantek Çelik]

(In reply to Mats Palmgren (:mats) from comment #15)
> This proposal was posted to www-style in Feb 2014 (by Tantek):
> http://lists.w3.org/Archives/Public/www-style/2014Feb/0140.html
> 
> Tab responded "Sounds reasonable to me".  There were no other responses.
> 
> I'm not sure if the spec has been updated, the opening sentence is still:
> "This property specifies rendering when inline content overflows its block
> container element"
> http://dev.w3.org/csswg/css-ui/#text-overflow
> for example, which seems wrong given the proposal.
> 
> Tantek, can I assume the proposal was accepted in the CSSWG?

Yes, there was no objection.


> When will the spec be updated?

Issue and resolution captured: https://wiki.csswg.org/spec/css3-ui#issue-32

Updating editor's draft next.


> BTW, is the revision history for this spec publicly accessible somewhere?
> It would be very useful to be able to see which edits have been made.

Yes it's on Mercurial on the W3C server.

https://dvcs.w3.org/hg/csswg/file/176c544cf1c5/css-ui

Comment 17

[Tantek Çelik]

Editor's draft updated to line box edge rather than block container edge.

http://dev.w3.org/csswg/css-ui/

Comment 20

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4aaeb4e14d195a020d5ad47c4b3637efd4a2332

Comment 21

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Both Chrome and Edge seems to have bugs with text-overflow.
Chrome breaks when using line-height:1 for some weird reason (the red cases).
It seems Edge only ellipses the first line in each block.

Filed this bug on Chrome:
https://bugs.chromium.org/p/chromium/issues/detail?id=710260

Comment 22

[Mats Palmgren (inactive)]

Attachment: Bug 944200 part 1 - [css-ui] Make TextOverflow::CanHaveTextOverflow not take a nsDisplayListBuilder so we can use it in Reflow; cache the value in BlockReflowInput::mCanHaveTextOverflow.

Review commit: https://reviewboard.mozilla.org/r/128714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128714/

Comment 23

[Mats Palmgren (inactive)]

Attachment: Bug 944200 part 2 - [css-ui] Store the line's float edges for text-overflow analysis if needed.

Review commit: https://reviewboard.mozilla.org/r/128716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128716/

Comment 24

[Mats Palmgren (inactive)]

Attachment: Bug 944200 part 3 - [css-ui] Refactor the code to prepare for narrowing the content area by the float edges in the next part (idempotent patch).

Review commit: https://reviewboard.mozilla.org/r/128718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128718/

Comment 25

[Mats Palmgren (inactive)]

Attachment: Bug 944200 part 4 - [css-ui] Use the stored float edges to narrow the block's content area to account for any floats on each line.

Review commit: https://reviewboard.mozilla.org/r/128720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128720/

Comment 26

[Mats Palmgren (inactive)]

Fairly straightforward implementation:
1. store aState.GetFloatAvailableSpaceForBSize for each line (if impacted by a float)
2. narrow the content area we're ellipsing to that space for each line,
   otherwise use the block's content rect as before if nothing was stored

Comment 27

[Mats Palmgren (inactive)]

> It seems Edge only ellipses the first line in each block.

Looks like they had a report on that but closed it as "Wontfix":
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/689773/
Wow, gotta love these guys. ;-)  Web standards? WTF?

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8856795 [details]
Bug 944200 part 1 - [css-ui] Make TextOverflow::CanHaveTextOverflow not take a nsDisplayListBuilder so we can use it in Reflow; cache the value in BlockReflowInput::mCanHaveTextOverflow.

https://reviewboard.mozilla.org/r/128714/#review131502

r=me with a comment added

::: layout/generic/TextOverflow.cpp:328
(Diff revision 1)
> -  if (!CanHaveTextOverflow(aBuilder, aBlockFrame)) {
> +  if (aBuilder->IsForEventDelivery() ||
> +      aBuilder->IsForFrameVisibility() ||
> +      !CanHaveTextOverflow(aBlockFrame)) {

Could you add a code-comment here (perhaps some form of the piece of the comment that's being removed in CanHaveTextOverflow), to explain this multi-part conditional?

(Right now, it's not clear why "IsForEventDelivery() || IsForFrameVisibility()" should make "WillProcessLines()" immediately bail.)

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8856796 [details]
Bug 944200 part 2 - [css-ui] Store the line's float edges for text-overflow analysis if needed.

https://reviewboard.mozilla.org/r/128716/#review131504

::: layout/generic/nsBlockFrame.cpp:2844
(Diff revision 1)
> +      WritingMode wm = aLine->mWritingMode;
> +      nsFlowAreaRect r = aState.GetFloatAvailableSpaceForBSize(aLine->BStart(),
> +                                                               aLine->BSize(),
> +                                                               nullptr);
> +      if (r.mHasFloats) {
> +        mozilla::LogicalRect so =

Let's drop the mozilla:: prefix here. (Elsewhere in this file, we just use "LogicalRect", because we have "using namespace mozilla".)

::: layout/generic/nsBlockFrame.cpp:2848
(Diff revision 1)
> +        if (so.IEnd(wm) > e || so.IStart(wm) < s) {
> +          aLine->SetFloatEdges(s, e);
> +        }

A comment would be helpful here, in part to hint/remind what the 1-or-2-letter variable names represent.

Maybe something like:
 // If scrollable overflow starts or ends outside of
 // the area that would be available for floats,
 // then we'll need to consider that area when doing
 // text-overflow analysis.

::: layout/generic/nsLineBox.h:700
(Diff revision 1)
> +      , mFloatEdgeIStart(NS_INTRINSIC_WIDTH_UNKNOWN)
> +      , mFloatEdgeIEnd(NS_INTRINSIC_WIDTH_UNKNOWN)

It feels wrong to use NS_INTRINSIC_WIDTH_UNKNOWN as our sentinel value here.  These variables are not intrinsic widths (or even widths at all) -- they're *positions* in the inline axis, AFAICT.

Is there another named sentinel that we could use here? Or perhaps we should just define a new one for usage here?

::: layout/generic/nsLineBox.h:708
(Diff revision 1)
> +  bool GetFloatEdges(nscoord* aStart, nscoord* aEnd) const {
> +    MOZ_ASSERT(IsInline(), "block line can't have float edges");
> +    const auto data = static_cast<ExtraInlineData*>(mData);
> +    if (data && data->mFloatEdgeIStart != NS_INTRINSIC_WIDTH_UNKNOWN) {
> +      *aStart = data->mFloatEdgeIStart;
> +      *aEnd = data->mFloatEdgeIEnd;

Can you just get rid of "data" & use mInlineData directly here? (That's what you do in the changes in the .cpp file, at least, and it seems cleaner. I don't see any other static_casts of mData anywhere.)

I think this will mean you need to wrap the conditional to 2 lines, but that's fine; it doesn't make the function any longer, since you'll be getting rid of the line for declaring the local variable.

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8856797 [details]
Bug 944200 part 3 - [css-ui] Refactor the code to prepare for narrowing the content area by the float edges in the next part (idempotent patch).

https://reviewboard.mozilla.org/r/128718/#review131536

::: layout/generic/TextOverflow.h:129
(Diff revision 1)
>     * and the bounds of what will be displayed between the markers.
>     * @param aLine the line we're processing
>     * @param aFramesToHide frames that should have their display items removed
>     * @param aAlignmentEdges the outermost edges of all text and atomic
>     *   inline-level frames that are inside the area between the markers
> +   * @return the area that we're ellipsing for this line

"the area that we're ellipsing" seems a bit too vague of a description. I'm not sure whether to interpret that as:
 - the area covered by the ellipsis marker.
 - the (potentially-large) area of overflowing text that is being replaced with the ellipsis marker.
 - ...or several other things.

Also: this function is about determining *whether* we need ellipses, it seems. In the event that we're not ellipsing anything, it's unclear what would be returned -- probably worth making that clearer in the documentation, too?

(Also: this new sentence would be the only mention of "ellipsing" [or any "ellips*" text] in this header file, FWIW.  Not sure why we don't mention ellipsis anywhere here, but it suggests that perhaps we were intending to keep the documentation marker-agnostic? Maybe that doesn't matter anymore, but it seemed notable.)

In any case: please reword in light of the above notes, to make the meaning clearer.

::: layout/generic/TextOverflow.h:202
(Diff revision 1)
>     * them into mMarkerList.
>     * @param aLine the line we're processing
>     * @param aCreateIStart if true, create a marker on the inline start side
>     * @param aCreateIEnd if true, create a marker on the inline end side
>     * @param aInsideMarkersArea is the area inside the markers
> +   * @param aContentArea is the content area inside any floats

"inside any floats" sounds like the internals of the floats themselves.

I think you mean *between* any floats, perhaps?  (Though, the wording should also make it clear what this arg represents in the event that there aren't any floats, since that's the normal case.)

::: layout/generic/TextOverflow.cpp:509
(Diff revision 1)
> +  // Save the non-snapped area since that's what we want to use when placing
> +  // the markers (our return value).  The snapped area is only for analysis.
> +  LogicalRect nonScrollSnappedContentArea = contentArea;
> +  if (mAdjustForPixelSnapping) {
> +    const nscoord scrollAdjust = mBlock->PresContext()->AppUnitsPerDevPixel();
> +    if (snapStart) {
> -  InflateIStart(mBlockWM, &contentArea, scrollAdjust);
> +      InflateIStart(mBlockWM, &contentArea, scrollAdjust);

This scrollAdjust / mAdjustForPixelSnapping / nonScrollSnappedContentArea reworking seems unrelated to the float-handling stuff here.  It seems like those changes should happen in a separate non-float-related changeset -- though I guess it's bundled into this changeset because it's part of having to return a LogicalRect from this function, and that's a change that's happening in this patch.

So: maybe we should split this patch in two, where:
 - the first part *just* focuses on making this function return a LogicalRect (and making callers respect that LogicalRect), without caring about floats yet -- i.e. it should preserve existing behavior.
 - the second part adds the aLine->GetFloatEdges() special case here (and that's the actual behavior change).

How does that sound?  That would make this easier to reason about for review as well as regression-hunting purposes, I think.

Comment 31

[Mats Palmgren (inactive)]

Attachment: Bug 944200 part 5 - [css-ui] Reftests for 'text-overflow' with text overlapping floats.

Review commit: https://reviewboard.mozilla.org/r/129162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129162/

Comment 32

[Mats Palmgren (inactive)]

Comment on attachment 8856795 [details]
Bug 944200 part 1 - [css-ui] Make TextOverflow::CanHaveTextOverflow not take a nsDisplayListBuilder so we can use it in Reflow; cache the value in BlockReflowInput::mCanHaveTextOverflow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128714/diff/1-2/

Comment 33

[Mats Palmgren (inactive)]

Comment on attachment 8856796 [details]
Bug 944200 part 2 - [css-ui] Store the line's float edges for text-overflow analysis if needed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128716/diff/1-2/

Comment 34

[Mats Palmgren (inactive)]

Comment on attachment 8856797 [details]
Bug 944200 part 3 - [css-ui] Refactor the code to prepare for narrowing the content area by the float edges in the next part (idempotent patch).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128718/diff/1-2/

Comment 35

[Mats Palmgren (inactive)]

Comment on attachment 8856798 [details]
Bug 944200 part 4 - [css-ui] Use the stored float edges to narrow the block's content area to account for any floats on each line.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128720/diff/1-2/

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 8856796 [details]
Bug 944200 part 2 - [css-ui] Store the line's float edges for text-overflow analysis if needed.

https://reviewboard.mozilla.org/r/128716/#review131736

r=me

::: commit-message-57285:1
(Diff revisions 1 - 2)
>  Bug 944200 part 2 - [css-ui] Store the line's float edges for text-overflow analysis if needed.  r=dholbert
>  
> -MozReview-Commit-ID: IdSsy7zVEUE
> +MozReview-Commit-ID: KqENMs07yrM

Note: it looks like some part of your update-patches-and-repush workflow caused the MozReview-Commit-ID to be regenerated/changed.

If you can figure out which of your steps caused that, it'd be worth avoiding/tweaking that step in the future so that this doesn't happen.  It's not a problem in this case -- but in scenarios where you're e.g. inserting new patches towards the beginning of an existing patch stack, the MozReview-Commit-ID is the only way for MozReview to correctly map old patch versions to (now-later-in-the-queue) new patch versions, & generate useful interdiffs.

Comment 37

[Daniel Holbert [:dholbert]]

Comment on attachment 8856797 [details]
Bug 944200 part 3 - [css-ui] Refactor the code to prepare for narrowing the content area by the float edges in the next part (idempotent patch).

https://reviewboard.mozilla.org/r/128718/#review131740

Thanks for splitting this up! r=me, modulo nits:

::: commit-message-4a18c:1
(Diff revision 2)
> +Bug 944200 part 3 - [css-ui] Refactor the code to prepare for narrowing the content area by the float edges in the next part (idempotent patch).  r=dholbert

I don't think this is actually idempotent, is it? (It's nearly idempotent, but not quite, IIUC.)

Specifically: this patch does s/mContentArea/contentArea/ in ExamineLineFrames, where the former variable isn't scroll snapped, but the latter is.  So I suspect this patch is fixing some bugs/inconsistencies around our analysis in that function, which could conceivably manifest as a small behavior change.

So -- if I've got that correct, you might [if you like] hint at the patch's nearly-idempotentness in a second explanatory line of the commit message rather than the main first line, and include a brief hint at the minor scroll snapping behavior change.

::: layout/generic/TextOverflow.h:132
(Diff revision 2)
> -  void ExamineLineFrames(nsLineBox*      aLine,
> +  mozilla::LogicalRect ExamineLineFrames(nsLineBox*      aLine,
> -                         FrameHashtable* aFramesToHide,
> +                                         FrameHashtable* aFramesToHide,
> -                         AlignmentEdges* aAlignmentEdges);
> +                                         AlignmentEdges* aAlignmentEdges);

This class has...
   typedef mozilla::LogicalRect LogicalRect;
...a little ways up from here, so you don't need the mozilla:: prefix on the type here.

(The function-decl above this also returns type "LogicalRect", without a mozilla prefix, by virtue of this typedef.)

::: layout/generic/TextOverflow.h:207
(Diff revision 2)
>                       bool aCreateIStart, bool aCreateIEnd,
> -                     const LogicalRect& aInsideMarkersArea);
> +                     const LogicalRect& aInsideMarkersArea,
> +                     const mozilla::LogicalRect& aContentArea);

Same here - no need for the prefix.

::: layout/generic/TextOverflow.cpp:468
(Diff revision 2)
> -void
> +mozilla::LogicalRect
>  TextOverflow::ExamineLineFrames(nsLineBox*      aLine,
>                                  FrameHashtable* aFramesToHide,

As above, drop the mozilla:: prefix.  (This .cpp file has "using namespace mozilla", and so nearly all other usages of LogicalRect in this .cpp file are unprefixed, so might as well match that.)

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8856798 [details]
Bug 944200 part 4 - [css-ui] Use the stored float edges to narrow the block's content area to account for any floats on each line.

https://reviewboard.mozilla.org/r/128720/#review131760

::: commit-message-b4ff7:1
(Diff revisions 1 - 2)
> -Bug 944200 part 4 - [css-ui] Reftests for 'text-overflow' with text overlapping floats.
> +Bug 944200 part 4 - [css-ui] Use the stored float edges to narrow the block's content area to account for any floats on each line.  r=dholbert
>  
> -MozReview-Commit-ID: 1LaeH2BjP8W
> +MozReview-Commit-ID: AFnDG8mQkiG

(Note: this actually shows the hypothetical problem that I brought up in comment 36, in practice -- due to the MozReview-Commit-IDs changing, it throws up its hands and shows an interdiff of "old patch#4 vs new patch#4", even though really we'd like it to match old patch#4 against new patch#5 for interdiffing purposes.

In this case it's not a big deal because the interdiffs between these patches aren't super relevant for review, but they could be useful in other cases.)

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8856798 [details]
Bug 944200 part 4 - [css-ui] Use the stored float edges to narrow the block's content area to account for any floats on each line.

https://reviewboard.mozilla.org/r/128720/#review131766

::: layout/generic/TextOverflow.cpp:494
(Diff revision 2)
> +  if (aLine->GetFloatEdges(&startEdge, &endEdge)) {
> +    // Narrow the |contentArea| to account for any floats on this line.
> +    nscoord delta = endEdge - contentArea.IEnd(mBlockWM);
> +    if (delta < 0) {
> +      nscoord newSize = contentArea.ISize(mBlockWM) + delta;
> +      contentArea.ISize(mBlockWM) = std::max(nscoord(0), newSize);
> +      snapEnd = false;

What's special about floats that make them need to disable the snapping codepath here?

(Superficially, it looks like the larger "snapped area" could trigger 1px of overflow on each edge in the no-floats scenario.  Why is that OK in that scenario, but not-OK in the floats scenario?)

Comment 40

[Mats Palmgren (inactive)]

> I don't think this is actually idempotent, is it? (It's nearly idempotent, but not quite, IIUC.)

You're right, good catch.  I'll make it idempotent as I intended by using
the non-snapped rect where it would have been used before.

I'm pretty sure we already have a reftest for this,
layout/reftests/text-overflow/xulscroll.html IIRC, but perhaps it got disabled
on the platform that mattered? or simply that the issue doesn't occur anymore
so we could remove this pixel-snapping quirk?  In any case, I'd prefer to
file a follow-up bug on that.

> What's special about floats that make them need to disable the snapping codepath here?

Just that I prefer to keep the quirk to the specific case it was intended for
and not extend it.  I don't think it's needed in the clip-against-a-float-edge
case.

Comment 41

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #38)
> (Note: this actually shows the hypothetical problem that I brought up in
> comment 36, in practice -- due to the MozReview-Commit-IDs changing, it
> throws up its hands and shows an interdiff of "old patch#4 vs new patch#4",
> even though really we'd like it to match old patch#4 against new patch#5 for
> interdiffing purposes.

Well, I did run "mach bootstrap" since the last time this happened but
apparently MQ isn't compatible with MozReview.
I'll stop using MozReview from now on, but please bare with me for this
bug since it already started in MozReview.

Comment 42

[Mats Palmgren (inactive)]

Comment on attachment 8856797 [details]
Bug 944200 part 3 - [css-ui] Refactor the code to prepare for narrowing the content area by the float edges in the next part (idempotent patch).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128718/diff/2-3/

Comment 43

[Mats Palmgren (inactive)]

Comment on attachment 8856798 [details]
Bug 944200 part 4 - [css-ui] Use the stored float edges to narrow the block's content area to account for any floats on each line.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128720/diff/2-3/

Comment 44

[Mats Palmgren (inactive)]

Comment on attachment 8857226 [details]
Bug 944200 part 5 - [css-ui] Reftests for 'text-overflow' with text overlapping floats.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129162/diff/1-2/

Comment 45

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd8659299a90c89c2077da12e3f7f4e0e6f18b75

Comment 46

[Daniel Holbert [:dholbert]]

Comment on attachment 8856798 [details]
Bug 944200 part 4 - [css-ui] Use the stored float edges to narrow the block's content area to account for any floats on each line.

https://reviewboard.mozilla.org/r/128720/#review132764

r=me, modulo the following:

::: layout/generic/TextOverflow.cpp:492
(Diff revision 3)
>    bool snapStart = true, snapEnd = true;
> +  nscoord startEdge, endEdge;
> +  if (aLine->GetFloatEdges(&startEdge, &endEdge)) {
> +    // Narrow the |contentArea| to account for any floats on this line.
> +    nscoord delta = endEdge - contentArea.IEnd(mBlockWM);
> +    if (delta < 0) {
> +      nscoord newSize = contentArea.ISize(mBlockWM) + delta;
> +      contentArea.ISize(mBlockWM) = std::max(nscoord(0), newSize);
> +      snapEnd = false;

Now that the snapping stuff is becoming optional here, could you document its intent a little more clearly here? I checked hg blame to try to learn more, but that takes me to bug 672944 which doesn't have any discussion of this quirk.  Specifically, could you:

(1) Add a comment above the snapStart/snapEnd decls, briefly explaining the quirk & what issue it's meant to avoid.

(2) Extend the comment inside the GetFloatEdges to mention that we're disabling the quirk -- e.g. maybe adding something like this:
    // and don't bother with snapping quirk on whichever side(s) we narrow.
...to make it clearer that the disabling is for convenience/simplicity rather than for some correctness reason (IIUC -- that's what I'm getting from comment 40, at least).

Comment 47

[Mats Palmgren (inactive)]

> (1) Add a comment above the snapStart/snapEnd decls, briefly explaining the quirk & what issue it's meant to avoid.

I think the quirk is sufficiently documented where mAdjustForPixelSnapping is setup:
http://searchfox.org/mozilla-central/rev/020510839be51489be1ba2f2e4e948b75a438d86/layout/generic/TextOverflow.cpp#277-290
That said, all the reftests pass with mAdjustForPixelSnapping=false for me locally
in a Linux64 debug build, so perhaps it's not needed anymore?  I'll submit a Try
job and see if this is the case on all platforms, and if so, file a bug to remove
this quirk.

> (2) Extend the comment inside the GetFloatEdges to mention that we're disabling the quirk

Added the line you suggested.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d1fe3994df
part 1 - [css-ui] Make TextOverflow::CanHaveTextOverflow not take a nsDisplayListBuilder so we can use it in Reflow; cache the value in BlockReflowInput::mCanHaveTextOverflow.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/32faa7c639ff
part 2 - [css-ui] Store the line's float edges for text-overflow analysis if needed.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a21b1caf9bf7
part 3 - [css-ui] Refactor the code to prepare for narrowing the content area by the float edges in the next part (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba55d6a84f0c
part 4 - [css-ui] Use the stored float edges to narrow the block's content area to account for any floats on each line.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b9f05ea3ad
part 5 - [css-ui] Reftests for 'text-overflow' with text overlapping floats.

Comment 49

[Mats Palmgren (inactive)]

Actually, layout/reftests/text-overflow/xulscroll.html does FAIL without
the mAdjustForPixelSnapping quirk (I made a mistake the first time I tried without it).

Comment 50

[Mats Palmgren (inactive)]

FTR, here's a better link for the mAdjustForPixelSnapping setup:
http://searchfox.org/mozilla-central/rev/020510839be51489be1ba2f2e4e948b75a438d86/layout/generic/TextOverflow.cpp#277-290,297-301

Comment 51

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a8d1fe3994df
https://hg.mozilla.org/mozilla-central/rev/32faa7c639ff
https://hg.mozilla.org/mozilla-central/rev/a21b1caf9bf7
https://hg.mozilla.org/mozilla-central/rev/ba55d6a84f0c
https://hg.mozilla.org/mozilla-central/rev/d5b9f05ea3ad

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: followup - Consistently use nscoord_MIN for uninitialized mFloatEdgeIStart/End

MozReview-Commit-ID: Ah9FUPf2dw4

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: followup - Consistently use nscoord_MIN for uninitialized mFloatEdgeIStart/End

This fixes an inconsistency in part 2 (32faa7c639ff), which wrote the
constant for uninitialized in two different ways.

The constructor uses nscoord_MIN for uninitialized, and that's also what
GetFloatEdges tests for, so this should use nscoord_MIN as well.

I believe this is because the review comment in comment 29 was only
partially addressed.

I noticed the difference because it caused broken behavior in a tree
with my patch for bug 1227493, which defines NS_INTRINSIC_WIDTH_UNKNOWN
differently.  Some text was disappearing on github (email and website
on user pages), slack (usernames of private chats in the sidebar), and
twitter (username of user who tweeted a quoted tweet).

MozReview-Commit-ID: Ah9FUPf2dw4

Comment 54

[Mats Palmgren (inactive)]

Comment on attachment 8868035 [details] [diff] [review]
followup - Consistently use nscoord_MIN for uninitialized mFloatEdgeIStart/End

Good catch, thanks.

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb689ff5ec1955ea014734027457a95af1764208
Bug 944200 followup - Consistently use nscoord_MIN for uninitialized mFloatEdgeIStart/End.  r=mats

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb689ff5ec19

Comment 57

[Joel Maher ( :jmaher ) (UTC -8)]

I see we added a reftest clause that compares the -ref to the -ref:
https://searchfox.org/mozilla-central/source/layout/reftests/text-overflow/reftest.list#28

and a file that is not a -ref was added, but not used:
https://searchfox.org/mozilla-central/source/layout/reftests/text-overflow/float-edges-1.html

:mats, can you help clarify why we have float-edges-1.html and it isn't used?  maybe we should use it or delete the file?

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f8b4c08b7f
(follow-up) - Fix typo in reftest manifest.  r=me DONTBUILD

Comment 59

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c5f8b4c08b7f