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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

: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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Test case

Load in the b2g browser and scroll.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 WIP - Add a flag to track APZ scrolling

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 WIP - Different attempts at hiding the ellipsis while scrolling

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 WIP - Hide ellipsis while actively scrolling by APZ

With lots of help from mstange got this to a working state. We might be able to do better though.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a flag to track APZ scrolling

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Invalidate when the char-clip edges change

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Don't ellipsize while APZ is scrolling

Not sure who all needs to review what, please free to bounce as needed.

Comment 9

[Markus Stange [: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?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 11

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Invalidate when the char-clip edges change

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

Comment 13

[Markus Stange [:mstange]]

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?

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Invalidate when the char-clip edges change

Good point, done.

Comment 15

[Timothy Nikkel (:tnikkel)]

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?

Comment 16

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 17

[Timothy Nikkel (:tnikkel)]

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.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Don't ellipsize while APZ is scrolling (v2)

Updated per comments 15-17.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/23b9a091e981
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/725f9800dbe8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/42b803b3895b

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23b9a091e981
https://hg.mozilla.org/mozilla-central/rev/725f9800dbe8
https://hg.mozilla.org/mozilla-central/rev/42b803b3895b

Comment 21

[Kartikaya Gupta (email:[e-mail])]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a3bf3ba5374c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/56db92353088
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ec57afdcdf81