Closed Bug 1400382 Opened 7 years ago Closed 7 years ago

text-layers: implement text-overflow: ellipsis

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: Gankra, Assigned: Gankra)

References

Details

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

Attachments

(4 files)

Currently we don't clip(?) the text properly, and we need to natively implement nsDisplayTextOverflowMarker's functionality (currently blobs).
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
Whiteboard: [wr-mvp] [triage]
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][gfx-noted]
Priority: -- → P2
Whiteboard: [wr-mvp] [triage][gfx-noted] → [wr-mvp] [gfx-noted]
Assignee: nobody → a.beingessner
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → mozilla57
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
Flags: needinfo?(mats)
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)
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!
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?
Whoops, good catch!
New patch reverts that; works great.
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 on attachment 8910751 [details]
Bug 1400382 - Defer TextDrawTarget analysis until GetLayerState.

https://reviewboard.mozilla.org/r/182216/#review187574
Attachment #8910751 - Flags: review?(jmuizelaar) → review+
Keywords: checkin-needed
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
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).
try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=719366d7ed06eb402402a2a800920eb6f2fbef24

(had to fix a couple switches to get rid of some warnings)
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 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 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+
All comments addressed (only whitespace changes). Annoyingly the rebase cleared mstange's r+'s...
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 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 on attachment 8910956 [details]
Bug 1400382 - Implement CreateWebRenderCommands for nsDisplayTextOverflow.

https://reviewboard.mozilla.org/r/182410/#review188088
Attachment #8910956 - Flags: review+
Keywords: checkin-needed
Target Milestone: mozilla57 → ---
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: