text-layers: implement text-overflow: ellipsis

RESOLVED FIXED in Firefox 58

Status

()

Core
Graphics: WebRender
P1
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: Gankro, Assigned: Gankro)

Tracking

(Blocks: 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 fixed)

Details

(Whiteboard: [wr-mvp] [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

8 months ago
Currently we don't clip(?) the text properly, and we need to natively implement nsDisplayTextOverflowMarker's functionality (currently blobs).
(Assignee)

Comment 1

8 months ago
Failing reftests associated with this (at least):

!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/ellipsis-font-fallback.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/ellipsis-font-fallback-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/line-clipping.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/line-clipping-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/marker-basic.html == http://localhost:59831/1505412836997/182/text-overflow/marker-basic-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/marker-string.html == http://localhost:59831/1505412836997/183/text-overflow/marker-string-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/bidi-simple.html == http://localhost:59831/1505412836998/184/text-overflow/bidi-simple-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/bidi-simple-scrolled.html == http://localhost:59831/1505412836999/185/text-overflow/bidi-simple-scrolled-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/false-marker-overlap.html == http://localhost:59831/1505412837000/188/text-overflow/false-marker-overlap-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/visibility-hidden.html == http://localhost:59831/1505412837001/189/text-overflow/visibility-hidden-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/block-padding.html == http://localhost:59831/1505412837002/190/text-overflow/block-padding-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/quirks-decorations.html == http://localhost:59831/1505412837002/191/text-overflow/quirks-decorations-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/quirks-line-height.html == http://localhost:59831/1505412837002/192/text-overflow/quirks-line-height-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/standards-decorations.html == http://localhost:59831/1505412837003/193/text-overflow/standards-decorations-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/standards-line-height.html == http://localhost:59831/1505412837003/194/text-overflow/standards-line-height-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/selection.html == http://localhost:59831/1505412837004/195/text-overflow/selection-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/marker-shadow.html == http://localhost:59831/1505412837004/196/text-overflow/marker-shadow-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/clipped-elements.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/clipped-elements-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/table-cell.html == http://localhost:59831/1505412837006/198/text-overflow/table-cell-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/two-value-syntax.html == http://localhost:59831/1505412837006/199/text-overflow/two-value-syntax-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/atomic-under-marker.html == http://localhost:59831/1505412837007/201/text-overflow/atomic-under-marker-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/combobox-zoom.html == http://localhost:59831/1505412837009/203/text-overflow/combobox-zoom-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/vertical-decorations-1.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/vertical-decorations-1-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/vertical-decorations-2.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/vertical-decorations-2-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/vertical-decorations-3.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/vertical-decorations-3-ref.html
!   file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/vertical-decorations-4.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-overflow/vertical-decorations-4-ref.html
Blocks: 1386665
status-firefox57: affected → unaffected

Updated

8 months ago
Whiteboard: [wr-mvp] [triage]

Updated

8 months ago
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][gfx-noted]

Updated

8 months ago
Priority: -- → P2
Whiteboard: [wr-mvp] [triage][gfx-noted] → [wr-mvp] [gfx-noted]
(Assignee)

Updated

8 months ago
Assignee: nobody → a.beingessner

Updated

8 months ago
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → mozilla57
(Assignee)

Comment 2

8 months ago
Hey mats, I hear you've got some knowledge here. I've been trying to figure out how TextOverflow/nsBlockFrame actually ends up clipping away the contents of an nsTextFrame/nsDisplayText for e.g. `text-overflow: ellipsis`. I made a simple example here:

<p style="text-wrap: none;  white-space: nowrap;">
  <span style="width: 5em; text-overflow: ellipsis; display: inline-block; overflow: hidden;">overflow text</span> 
</p>


But when I dump the display list with layout.display-list.dump-content, it says the elements overlap and have identical clipping, indicating that clipping doesn't seem to actually be involved at all? How does it work?


Text         p=0x10b9d7320 f=0x121430ed8() bounds(360,996,5329,1080) layerBounds(360,996,5329,1080) visible(480,960,4800,1152) componentAlpha(360,996,5329,1080) clip(480,960,4800,1152) asr(<0x1214301d8>) clipChain(<480,960,4800,1152> [0x1214301d8], <0,0,76800,58080> [root asr]) ref=0x121430018 agr=0x1214300a8 layer=0x10fe52400

TextOverflow p=0x10b9d73a8 f=0x121430e28() bounds(4131,960,960,1152) layerBounds(4131,960,960,1152) visible(480,960,4800,1152) componentAlpha(4131,960,960,1152) clip(480,960,4800,1152) asr(<0x1214301d8>) clipChain(<480,960,4800,1152> [0x1214301d8], <0,0,76800,58080> [root asr]) ref=0x121430018 agr=0x1214300a8 layer=0x10fe52400
(Assignee)

Updated

8 months ago
Flags: needinfo?(mats)

Comment 3

8 months ago
Yeah, normal clipping isn't involved.  nsBlockFrame generates display items
as usual, TextOverflow then analyzes the items on each line and simply
removes the items that overflows the content box (shortened by floats).
Items for text that spans the edge are kept though and are handled by
narrowing the edges of the nsCharClipDisplayItem (mVisI[Start|End]Edge).
see TextOverflow::PruneDisplayListContents
nsTextFrame::PaintText then displays "characters" (grapheme clusters) that
are fully visible within that range and skips the rest.
see nsTextFrame::MeasureCharClippedText

Using normal clipping would be wrong per spec, since we should remove "atomic"
boxes completely unless they fit fully inside, and text should be removed
character-by-character until it fits (together with the ellipsis).
https://drafts.csswg.org/css-ui-4/#propdef-text-overflow

Does that help?
Flags: needinfo?(mats)
(Assignee)

Comment 4

8 months ago
Oh I see, clipping would be wrong if two characters overlapped, and only the second one was overflowing.

nsCharClipDisplayItem was the missing piece of the puzzle, thanks!
Comment hidden (mozreview-request)
(Assignee)

Comment 6

8 months ago
Attached patch fixes our rendering, but still leaves the "..." to be rendered by gecko. I briefly looked into implementing nsDisplayTextOverflowMarker::CreateWebRenderCommands, but this somehow manages to feed through tons of new code that nsDisplayText doesn't, and I wanted to spend some more time thinking about my approach.
Does the patch here make the patch from bug 1395098 unnecessary? With your current patch, does folded-down opacity get applied twice?
(Assignee)

Comment 8

8 months ago
Whoops, good catch!
Comment hidden (mozreview-request)
(Assignee)

Comment 10

8 months ago
New patch reverts that; works great.
(Assignee)

Comment 11

8 months ago
With this patch all the reftests I posted above are good to the human eye, but need fuzzing for subpixel differences until wr is drawing the ellipses.

Comment 12

8 months ago
mozreview-review
Comment on attachment 8910751 [details]
Bug 1400382 - Defer TextDrawTarget analysis until GetLayerState.

https://reviewboard.mozilla.org/r/182216/#review187574
Attachment #8910751 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

8 months ago
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

8 months ago
Ok, since this had a rebase conflict I took the opportunity to do the full job here.

There's now 4 patches:

* compute TextDrawTarget later (clips the text properly)
* refactor text code to use casts instead of explicit passing of TextDrawTarget (significant cleanup)
* refactor the WebRenderCommand building code to live in TextDrawTarget (mostly just copy-paste from one file to other)
* implement CreateWebRenderCommands for nsDisplayTextOverflow (nearly trivial given the above work)

Pretty happy with this patchset!
Keywords: checkin-needed
(Assignee)

Comment 18

8 months ago
All of the above reftests should now pass without fuzzing (looked good live, haven't tried rebasing on top of layers free to run in try).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

8 months ago
try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=719366d7ed06eb402402a2a800920eb6f2fbef24

(had to fix a couple switches to get rid of some warnings)

Comment 24

8 months ago
mozreview-review
Comment on attachment 8910954 [details]
Bug 1400382 - Replace explicit TextDrawTarget passing with cast-based system.

https://reviewboard.mozilla.org/r/182406/#review188064

::: gfx/thebes/gfxContext.cpp:129
(Diff revision 2)
>      }
>    }
>  }
>  
> +mozilla::layout::TextDrawTarget* 
> +gfxContext::GetTextDrawer() 

some end-of-line whitespace here

::: layout/generic/TextDrawTarget.h:10
(Diff revision 2)
>  
>  #ifndef TextDrawTarget_h
>  #define TextDrawTarget_h
>  
>  #include "mozilla/gfx/2D.h"
> +#include "mozilla/webrender/WebRenderApi.h"

Why is this include needed?

::: layout/generic/nsTextFrame.cpp:6786
(Diff revision 2)
>      pt.y = (aParams.textBaselinePt.y - mAscent) / app;
>    }
>    gfxFloat decorationOffsetDir = mTextRun->IsSidewaysLeft() ? -1.0 : 1.0;
>    SelectionType nextSelectionType;
>    TextRangeStyle selectedStyle;
> +  

end-of-line whitespace
Attachment #8910954 - Flags: review+

Comment 25

8 months ago
mozreview-review
Comment on attachment 8910955 [details]
Bug 1400382 - Factor out text WebRenderCommand code to TextDrawTarget.

https://reviewboard.mozilla.org/r/182408/#review188066

::: layout/generic/TextDrawTarget.h:313
(Diff revision 2)
> +                          const layers::StackingContextHelper& aSc,
> +                          layers::WebRenderLayerManager* aManager,
> +                          nsDisplayItem* aItem,
> +                          nsRect& aBounds) {
> +
> +  // Drawing order: selections, 

end-of-line whitespace

::: layout/generic/TextDrawTarget.h:324
(Diff revision 2)
> +  // Compute clip/bounds
> +  auto appUnitsPerDevPixel = aItem->Frame()->PresContext()->AppUnitsPerDevPixel();
> +  LayoutDeviceRect layoutBoundsRect = LayoutDeviceRect::FromAppUnits(
> +      aBounds, appUnitsPerDevPixel);
> +  LayoutDeviceRect layoutClipRect = layoutBoundsRect;
> +  auto clip = aItem->GetClip(); 

end-of-line whitespace
Attachment #8910955 - Flags: review+

Comment 26

8 months ago
mozreview-review
Comment on attachment 8910956 [details]
Bug 1400382 - Implement CreateWebRenderCommands for nsDisplayTextOverflow.

https://reviewboard.mozilla.org/r/182410/#review188068

::: layout/base/nsLayoutUtils.cpp:6090
(Diff revision 3)
> +      };
> +
> +      wrShadow.blur_radius = presCtx->AppUnitsToFloatDevPixels(shadowDetails->mRadius);
> +      wrShadow.color = wr::ToColorF(ToDeviceColor(shadowColor));
> +
> +      textDrawer->AppendShadow(wrShadow);

So this assumes that textDrawer->mCurrentPart is exactly the text that aCallback would draw. That seems a bit brittle, but I guess it's ok for now...

::: layout/base/nsLayoutUtils.cpp:6101
(Diff revision 3)
>                                                      aDestCtx, aDirtyRect, nullptr,
>                                                      nsContextBoxBlur::DISABLE_HARDWARE_ACCELERATION_BLUR);
>      if (!shadowContext)
>        continue;
>  
> -    nscolor shadowColor;
> +    

end-of-line whitespace

::: layout/generic/TextOverflow.cpp:212
(Diff revision 3)
>    virtual uint32_t GetPerFrameKey() const override {
>      return (mIndex << TYPE_BITS) | nsDisplayItem::GetPerFrameKey();
>    }
>    void PaintTextToContext(gfxContext* aCtx,
>                            nsPoint aOffsetFromRect);
> +  

end-of-line whitespace

::: layout/generic/TextOverflow.cpp:294
(Diff revision 3)
> +bool
> +nsDisplayTextOverflowMarker::CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,
> +                                                     mozilla::wr::IpcResourceUpdateQueue& aResources,
> +                                                     const StackingContextHelper& aSc,
> +                                                     layers::WebRenderLayerManager* aManager,
> +                                                     nsDisplayListBuilder* aDisplayListBuilder) 

end-of-line whitespace

::: layout/generic/TextOverflow.cpp:318
(Diff revision 3)
> +  Paint(aDisplayListBuilder, captureCtx);
> +
> +  if (!textDrawer->CanSerializeFonts()) {
> +    return false;
> +  }
> +  

end-of-line whitespace
Attachment #8910956 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

8 months ago
All comments addressed (only whitespace changes). Annoyingly the rebase cleared mstange's r+'s...

Comment 32

8 months ago
mozreview-review
Comment on attachment 8910954 [details]
Bug 1400382 - Replace explicit TextDrawTarget passing with cast-based system.

https://reviewboard.mozilla.org/r/182406/#review188084
Attachment #8910954 - Flags: review+
Attachment #8910954 - Flags: review?(jmuizelaar)
Attachment #8910955 - Flags: review?(jmuizelaar)
Attachment #8910956 - Flags: review?(jmuizelaar)

Comment 33

8 months ago
mozreview-review
Comment on attachment 8910955 [details]
Bug 1400382 - Factor out text WebRenderCommand code to TextDrawTarget.

https://reviewboard.mozilla.org/r/182408/#review188086
Attachment #8910955 - Flags: review+

Comment 34

8 months ago
mozreview-review
Comment on attachment 8910956 [details]
Bug 1400382 - Implement CreateWebRenderCommands for nsDisplayTextOverflow.

https://reviewboard.mozilla.org/r/182410/#review188088
Attachment #8910956 - Flags: review+
(Assignee)

Updated

8 months ago
Keywords: checkin-needed
Target Milestone: mozilla57 → ---

Comment 35

8 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0eb23aabce86
Defer TextDrawTarget analysis until GetLayerState. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/87097033ee7b
Replace explicit TextDrawTarget passing with cast-based system. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a30dd89e6d07
Factor out text WebRenderCommand code to TextDrawTarget. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8ac3b543af39
Implement CreateWebRenderCommands for nsDisplayTextOverflow. r=mstange
Keywords: checkin-needed

Comment 36

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0eb23aabce86
https://hg.mozilla.org/mozilla-central/rev/87097033ee7b
https://hg.mozilla.org/mozilla-central/rev/a30dd89e6d07
https://hg.mozilla.org/mozilla-central/rev/8ac3b543af39
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.