Closed
Bug 1400382
Opened 7 years ago
Closed 7 years ago
text-layers: implement text-overflow: ellipsis
Categories
(Core :: Graphics: WebRender, enhancement, P1)
Core
Graphics: WebRender
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).
Assignee | ||
Comment 1•7 years 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
Updated•7 years ago
|
Blocks: stage-wr-nightly
Updated•7 years ago
|
Whiteboard: [wr-mvp] [triage]
Updated•7 years ago
|
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][gfx-noted]
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: [wr-mvp] [triage][gfx-noted] → [wr-mvp] [gfx-noted]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → a.beingessner
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → mozilla57
Assignee | ||
Comment 2•7 years 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•7 years ago
|
Flags: needinfo?(mats)
Comment 3•7 years 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•7 years 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•7 years 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.
Comment 7•7 years ago
|
||
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•7 years ago
|
||
Whoops, good catch!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
New patch reverts that; works great.
Assignee | ||
Comment 11•7 years 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•7 years 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•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
All comments addressed (only whitespace changes). Annoyingly the rebase cleared mstange's r+'s...
Comment 32•7 years 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+
Updated•7 years ago
|
Attachment #8910954 -
Flags: review?(jmuizelaar)
Updated•7 years ago
|
Attachment #8910955 -
Flags: review?(jmuizelaar)
Attachment #8910956 -
Flags: review?(jmuizelaar)
Comment 33•7 years 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•7 years 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•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Target Milestone: mozilla57 → ---
Comment 35•7 years 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•7 years 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
Closed: 7 years 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.
Description
•