text-overflow:ellipsis elements don't async scroll properly

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla39
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(4 attachments, 6 obsolete attachments)

348 bytes, text/html
Details
6.52 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.51 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.80 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
:mstange said that as part of getting APZC running on OS X he ran into a problem with text-overflow:ellipsis elements. Async scrolling these elements means the ellipsis (which is put in at paint time on the main thread) will not stay where the user expects it to. This is probably broken on Firefox OS right now.

Botond suggested that a good fix for this would be to hide the ellipsis while the element is actively scrolling and then put them back once the scrolling stops. roc said this would be an easy two-line fix to just not ellipsize if the element is actively scrolled.
Posted file Test case
Load in the b2g browser and scroll.
Component: Panning and Zooming → Layout
Mentor: bugmail.mozilla
Assignee: nobody → bugmail.mozilla
Mentor: botond, bugmail.mozilla
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)
> Botond suggested that a good fix for this would be to hide the ellipsis
> while the element is actively scrolling and then put them back once the
> scrolling stops. roc said this would be an easy two-line fix to just not
> ellipsize if the element is actively scrolled.

So I tried implementing this, and I was able to make the ellipsis appear and disappear on demand. However, the text being scrolled doesn't get painted for the most part. AFAICT the problem is that while the ellipsis is visible (e.g. on the initial render of the page) the code at [1] clips the text so that only the portion up to the ellipsis gets rendered. However, layout considers the entire thing to be "rendered" and then in the future only ever invalidates the bit at the end where the ellipsis appears/disappears. So the rest of the text never actually gets drawn. Still poking around to see if I can find a good fix.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/TextOverflow.cpp?rev=097629b2eb2a#659
With lots of help from mstange got this to a working state. We might be able to do better though.
Attachment #8568178 - Attachment is obsolete: true
Not sure who all needs to review what, please free to bounce as needed.
Attachment #8568620 - Attachment is obsolete: true
Attachment #8568696 - Flags: review?(tnikkel)
Attachment #8568696 - Flags: review?(mstange)
Comment on attachment 8568695 [details] [diff] [review]
Part 2 - Invalidate when the char-clip edges change

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

Looks good with the comments addressed, but somebody else should sign off on the patch since most of the ideas came from me.

::: layout/base/nsDisplayList.h
@@ +3639,5 @@
> +  nsCharClipGeometry(nsDisplayItem* aItem, nsDisplayListBuilder* aBuilder,
> +                     nscoord aLeftEdge, nscoord aRightEdge)
> +    : nsDisplayItemGenericGeometry(aItem, aBuilder)
> +    , mLeftEdge(aLeftEdge)
> +    , mRightEdge(aRightEdge)

You can just pull these two from aItem->mLeftEdge/mRightEdge.

@@ +3668,5 @@
>      : nsDisplayItem(aFrame) {}
>  
> +  virtual nsDisplayItemGeometry* AllocateGeometry(nsDisplayListBuilder* aBuilder) MOZ_OVERRIDE
> +  {
> +    return new nsCharClipGeometry(this, aBuilder, mLeftEdge, mRightEdge);

so you don't have to pass the fields here

::: layout/generic/nsTextFrame.cpp
@@ +4630,5 @@
>      nsRect newRect = geometry->mBounds;
>      nsRect oldRect = GetBounds(aBuilder, &snap);
>      if (decorations != geometry->mDecorations ||
> +        mLeftEdge != geometry->mLeftEdge ||
> +        mRightEdge != geometry->mRightEdge ||

Yeah, it really would be nicer if we were just calling the superclass method here. File a bug?
Attachment #8568695 - Flags: review?(mstange) → review+
Attachment #8568696 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #9)
> > +  nsCharClipGeometry(nsDisplayItem* aItem, nsDisplayListBuilder* aBuilder,
> > +                     nscoord aLeftEdge, nscoord aRightEdge)
> > +    : nsDisplayItemGenericGeometry(aItem, aBuilder)
> > +    , mLeftEdge(aLeftEdge)
> > +    , mRightEdge(aRightEdge)
> 
> You can just pull these two from aItem->mLeftEdge/mRightEdge.
> 

Sounds good. Presumably I'd have to static_cast the aItem to get those fields but that's fine.

> 
> Yeah, it really would be nicer if we were just calling the superclass method
> here. File a bug?

Filed bug 1136323 for this.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> (In reply to Markus Stange [:mstange] from comment #9)
> > > +  nsCharClipGeometry(nsDisplayItem* aItem, nsDisplayListBuilder* aBuilder,
> > > +                     nscoord aLeftEdge, nscoord aRightEdge)
> > > +    : nsDisplayItemGenericGeometry(aItem, aBuilder)
> > > +    , mLeftEdge(aLeftEdge)
> > > +    , mRightEdge(aRightEdge)
> > 
> > You can just pull these two from aItem->mLeftEdge/mRightEdge.
> > 
> 
> Sounds good. Presumably I'd have to static_cast the aItem to get those
> fields but that's fine.

Just make aItem of type nsCharClipDisplayItem*, "this" in AllocateGeometry is of that type.

> > Yeah, it really would be nicer if we were just calling the superclass method
> > here. File a bug?
> 
> Filed bug 1136323 for this.

thanks
Updated part 2 as per Markus' comments. I had to rearrange some stuff for the compiler to be happy with declaration order but fundamentally it's the same.

Also did a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ef5e0b2b1ef
Attachment #8568695 - Attachment is obsolete: true
Attachment #8568695 - Flags: review?(matt.woodrow)
Attachment #8568760 - Flags: review?(matt.woodrow)
Hmm, this makes nsCharClipGeometry the only geometry that's in nsDisplayList.h. Can you move it back to nsDisplayListInvalidation.h and move the definition of the constructor into nsDisplayListInvalidation.cpp?
Good point, done.
Attachment #8568760 - Attachment is obsolete: true
Attachment #8568760 - Flags: review?(matt.woodrow)
Attachment #8568766 - Flags: review?(matt.woodrow)
Attachment #8568694 - Flags: review?(tnikkel) → review+
Comment on attachment 8568696 [details] [diff] [review]
Part 3 - Don't ellipsize while APZ is scrolling

I'd prefer if we could avoid a useless paint in the common case that we don't have textoverflow. Can we do basic style checks like in TextOverflow::CanHaveTextOverflow before asking for a paint?
Attachment #8568696 - Flags: review?(tnikkel)
In the common case when this flag flips we're going to be doing paints anyway to show/hide the scrollbar. That being said I don't mind checking for NS_STYLE_TEXT_OVERFLOW_CLIP but anything more than that feels like unnecessary code duplication.
Oh good point. May as well check the style since it's quick. I wasn't suggesting duplicating the code, more like refactoring it so it can be used.
Attachment #8568766 - Flags: review?(matt.woodrow) → review+
Updated per comments 15-17.
Attachment #8568696 - Attachment is obsolete: true
Attachment #8568949 - Flags: review?(tnikkel)
Attachment #8568949 - Flags: review?(tnikkel) → review+
Comment on attachment 8568694 [details] [diff] [review]
Part 1 - Add a flag to track APZ scrolling

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): APZ
User impact if declined: when scrolling content with text-overflow:ellipsis the ellipsis jitters and the content sometimes doesn't render properly (see bug 1134504 which is an example of this, and marked 2.2+).
Testing completed: locally
Risk to taking this patch (and alternatives if risky): pretty low risk, I think. At worst we might end up doing a few extra repaints here and there but I don't think it will have a noticeable impact.
String or UUID changes made by this patch: none
Attachment #8568694 - Flags: approval-mozilla-b2g37?
Attachment #8568766 - Flags: approval-mozilla-b2g37?
Attachment #8568949 - Flags: approval-mozilla-b2g37?
No longer blocks: 1134504
Duplicate of this bug: 1134504
Attachment #8568694 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8568766 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8568949 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.