990974 - Unexpected checkerboarding when scrolling input field in Contacts app

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Botond Ballo [:botond]]

As described in https://bugzilla.mozilla.org/show_bug.cgi?id=984490#c27, scrolling the phone number input field on the New Contact page of the Contacts app (after typing enough numbers to make the field scrollable) results in checkerboarding even though the input field's scrollable layer has a displayport that covers the whole scrollable rect.

This is unexpected and a poor experience for the user, so we should investigate this.

Comment 1

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

Note that in this case the displayport is being set correctly (as far as I can tell) so maybe the problem is that input fields don't respect the displayport? The code path being taken might be different from scrollframes.

Comment 2

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

Does this still happen?

Comment 3

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Does this still happen?

Just checked with a recent trunk. It still happens.

Comment 4

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

tn, do you know if input fields also use nsGfxScrollFrame?

Comment 5

[Timothy Nikkel (:tnikkel)]

Yes, I think they do.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> tn, do you know if input fields also use nsGfxScrollFrame?

They do. The call to IsFocused when determining whether to layerize a scroll frame [1] was added specifically for the benefit of input fields in bug 946408.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=f998a8695d75#1031

Comment 7

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

Attachment: Standalone test case

The div scrolls without checkerboarding, but the input element has checkerboarding. They should really behave the same.

Comment 8

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

Attachment: Layer dump for test case

The visible region for the ClientTiledThebesLayer corresponding to the <input> is only 300px wide whereas the visible region for the <div> is 816px wide (presumably that's the width of the actual text). This probably explains why we don't paint in the rest of the input.

Comment 9

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

Attachment: Layer dump for test case

Now with display list as well. It looks like there is a clip(600,1920,18000,1320) set on the text item inside the input but an empty clip on the text item inside the div.

Comment 10

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

I'm assuming the clip is coming from http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css?rev=88c58ffd8e5f#72 or something similar. Haven't confirmed that yet.

Comment 11

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

And indeed, commenting out all of the overflow-clip-box properties in that file fixes the problem. tn, do you know what that property is supposed to be accomplishing?

Comment 12

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

Attachment: One way of fixing this problem

Comment 13

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

Bug 965237 added this earlier this year. CC'ing mats also but he's on vacation.

Comment 14

[Timothy Nikkel (:tnikkel)]

Talked to Mats on irc, seems like the block of code that applies overflow-clip-box: content for scrollframes here http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2828 should be done after we wrap the scrolled content in scroll layer items so that we don't clip the scrolled content drawn inside the scroll frame, but we do clip it when drawing it to the screen.

Comment 15

[Mats Palmgren (inactive)]

FYI, there's a pref, layout.css.overflow-clip-box.enabled, that enables
the property for arbitrary content so you can test it on a <div> for example.
(otherwise, it's restricted to the UA stylesheet. <input> and <textarea> has
overflow-clip-box:content-box in the UA sheet)

Comment 16

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

Sounds like it would make a good mentored bug for layout. If neither one of you wants to mentor it (or fix it) I can try to just write a patch.

Comment 17

[Timothy Nikkel (:tnikkel)]

I can mentor, patch, or review. :)

Comment 18

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

This isn't particularly high priority so let's go with mentoring :)

Comment 19

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

Hey Nick, are you working on this? If so, great! Otherwise it might be better to leave this unassigned so other people can work on it.

Comment 20

[Nick Lebedev [:nl]]

Yes, I would like to work on this bug. Could you briefly tell me what would be a good way to start on it.
(I read the comments but didn't got final and clear understanding what is the good approach here to fix it).

Comment 21

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

tn ^

Comment 22

[Timothy Nikkel (:tnikkel)]

Since comment 14, bug 967844 has landed, and that has changed what we need to do to fix this.

I think what we need to do now to fix this is modify the code here
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=2dc71497e243#2956

We normally clip the scrolled content to the padding box. Unless the overflow clip box is set to the content box (https://developer.mozilla.org/en-US/docs/Web/CSS/box_model). https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-clip-box has an example of this. The linked code implements this clipping. To implement async scrolling we need to paint more of the scrolled content then is visible at any one time. So we can't clip the content to what's currently visible. So if we are drawing an async scrollable scroll frame we don't clip the scrolled content when drawing it, but we then tell FrameLayerBuilder what clip needs to be used to composite the scrolled content to the screen in ScrollFrameHelper::ComputeFrameMetrics. So we need to inset the scrollport by the padding in ScrollFrameHelper::ComputeFrameMetrics when we use it as the clip. And we also want to skip applying the clip at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=2dc71497e243#2956 if mShouldBuildScrollableLayer is true.

Does that help?

Comment 23

[Nick Lebedev [:nl]]

Attachment: Stop clipping rendered area of the input fields in scollable layers

Yes, seems like I got your idea. Could you take a look please at my patch and tell me whether I'm moving in right direction.

Btw, I didn't test it much, I'm going to do it after bug 969466 is done.

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8511990 [details] [diff] [review]
Stop clipping rendered area of the input fields in scollable layers

This looks good, but I realized there might be a problem with list because we don't always clip the caret the same as we clip the rest of the content.

Comment 25

[Timothy Nikkel (:tnikkel)]

Mats, we have some code that doesn't clip the caret to the content box in some situations. This causes a problem when we want to async scroll the scroll frame. We render extra content outside of the scrollport (so it's not clipped by the scrollport). Then the compositor can move this content, and then finally apply the scrollport clip rect. If the scrolled content and the caret are in the same layer there is no way to clip the caret and the other content differently. How important is this different clipping for the caret? Is it something we could forgo in async scrolling situations?

Comment 26

[Mats Palmgren (inactive)]

> How important is this different clipping for the caret?

Very important.  The caret isn't visible in common use cases without it.
Typing at the end of an <input> where the text overflows for example.

> Is it something we could forgo in async scrolling situations?

Maybe.  As long as the caret shows up again when the scrolling stops
then it might be acceptable.  Maybe we can force a caret on-cycle
to show it sooner?

Alternatively, maybe we can force the caret display position to be
inside the content area while a scroll is in motion?  (I think I like
this better if it's possible.)

Comment 27

[Timothy Nikkel (:tnikkel)]

Unfortunately the clip we use is used at all times, during an async scroll, and when at a steady state.

Is it possible to force the caret position to be inside the content area in general?

The only other idea I had is to put the caret in its own layer and add the machinery to give it a different clip, but I'd prefer your idea.

Comment 28

[Mats Palmgren (inactive)]

> Is it possible to force the caret position to be inside the content area in general?

Unfortunately, I don't think so.  I think we did that at some point and had to revert
it because people didn't like the caret to overlap the character they just typed.
We still do it for padding:0 though, so you can try it and see for yourself:
data:text/html,<input style=padding:0>

Perhaps roc have some fresh ideas?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the caret is going to have to go in its own layer. We want to do that anyway so we can animate caret blinking in the compositor.

Comment 30

[Timothy Nikkel (:tnikkel)]

Okay. I found bug 866731 which looks to be about layerizing the caret and doing the animation OMT, so I made it block this bug.

Nick, thanks for your patch! It implements the idea I had to fix this bug, but unfortunately I realized that my idea wasn't quite right and we'll need other changes before we can tackle this.

Comment 31

[Nick Lebedev [:nl]]

(In reply to Timothy Nikkel (:tn) from comment #30)
> Okay. I found bug 866731 which looks to be about layerizing the caret and
> doing the animation OMT, so I made it block this bug.
> 
> Nick, thanks for your patch! It implements the idea I had to fix this bug,
> but unfortunately I realized that my idea wasn't quite right and we'll need
> other changes before we can tackle this.

Ok, could you let me know please if you come up to another idea when blocker is fixed. Maybe I could implement it too.

Comment 33

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

tn's comment copied over from bug 1133588 comment 23:

> The problem here is that the textarea has overflow-clip-box: content-box
> (from forms.css here
> http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.
> css?rev=6267c4d1de60#124 ) so the code here
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.cpp?rev=4604104f73a8#2984 goes through the list of scrolled
> content and clips it all (after we carefully didn't clip it because we had a
> displayport). The clipping treats carets specially, we've come across this
> in another bug before.
> 
> The easiest fix off the top of my head: mark caret items that need this
> special clipping from ScrollFrameHelper::BuildDisplayList, hack
> FrameLayerBuilder to always put these marked caret items in their own
> painted layer, hack the ComputeFrameMetrics call for this special caret
> painted layer to do special clip handling. This still doesn't take into
> account that the exact clipping behaviour currently coded for carets in this
> situation will change during async scroll, but perhaps it's not too bad.

Comment 34

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

So for this we're going to need the caret to *always* get layerized, right? The updated patch I attached to bug 866731 will layerize the caret if there's no "hook rect" on it. But that's not the same as always. It should be possible to layerize it into a PaintedLayer if there is a hook rect, I think.

Comment 35

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 990974 - Use variadic templates for ClipListsExceptCaret.

Bug 990974 - Use variadic templates for ClipListsExceptCaret.

Comment 36

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 990974 - Slightly change how the caret clip works.

Bug 990974 - Slightly change how the caret clip works.

Instead of looking at the caret's rect and determining whether we should clip it
to the real content box clip or not at all, we just always clip it, but to a
slightly bigger rect. In the cases that the caret was completely visible before,
it'll still be completely visible with this change. However, in the cases that we
did decide to clip before this patch, the result can be slightly different now:
Before this patch, whenever the caret was partially clipped, it was partially
clipped to the true content clip rect, but with this patch, the caret can be
partially clipped to the slightly larger clip rect.

Comment 37

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 990974 - When using display ports, don't clip the painted contents to the content box clip.

Bug 990974 - When using display ports, don't clip the painted contents to the content box clip.

Store the content box clip on mAncestorClip, and store a different clip for the caret on mAncestorClipForCaret.
In a future patch, those clips will be selectively applied to the right layers.

Comment 38

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 990974 - Add a flag to ComputeFrameMetrics so that a different clip can be returned for the caret and non-caret content.

Bug 990974 - Add a flag to ComputeFrameMetrics so that a different clip can be returned for the caret and non-caret content.

Comment 39

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 990974 - Give PaintedLayerDataTree a generic way of restricting a PaintedLayer to exactly one display item.

Bug 990974 - Give PaintedLayerDataTree a generic way of restricting a PaintedLayer to exactly one display item.

Comment 40

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 990974 - Treat carets in async scrolled scrollframes differently from non-caret content.

Bug 990974 - Treat carets in async scrolled scrollframes differently from non-caret content.

This makes those caret display items not share a layer with any other display items, and uses a different async scroll frame clip for that layer.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8629052 [details]
MozReview Request: Bug 990974 - Use variadic templates for ClipListsExceptCaret.

https://reviewboard.mozilla.org/r/12509/#review11027

Ship It!

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8629053 [details]
MozReview Request: Bug 990974 - Slightly change how the caret clip works.

https://reviewboard.mozilla.org/r/12511/#review11029

Ship It!

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8629054 [details]
MozReview Request: Bug 990974 - When using display ports, don't clip the painted contents to the content box clip.

https://reviewboard.mozilla.org/r/12513/#review11031

Ship It!

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8629055 [details]
MozReview Request: Bug 990974 - Add a flag to ComputeFrameMetrics so that a different clip can be returned for the caret and non-caret content.

https://reviewboard.mozilla.org/r/12515/#review11033

Ship It!

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8629056 [details]
MozReview Request: Bug 990974 - Give PaintedLayerDataTree a generic way of restricting a PaintedLayer to exactly one display item.

https://reviewboard.mozilla.org/r/12517/#review11035

Ship It!

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8629057 [details]
MozReview Request: Bug 990974 - Treat carets in async scrolled scrollframes differently from non-caret content.

https://reviewboard.mozilla.org/r/12519/#review11037

Ship It!

Comment 47

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> Comment on attachment 8629052 [details]
> MozReview Request: Bug 990974 - Use variadic templates for
> ClipListsExceptCaret.

I expected some pushback on this patch, and after discussing with BenWa and jrmuizel we decided that it wasn't worth the benefits and added mental overhead, so I'm going to drop this part.

Comment 48

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0a3fbad027
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b7333d90fc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/edc67a198700
https://hg.mozilla.org/integration/mozilla-inbound/rev/554f19622867
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5a47613fe6

Comment 49

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8d0a3fbad027
https://hg.mozilla.org/mozilla-central/rev/0b7333d90fc1
https://hg.mozilla.org/mozilla-central/rev/edc67a198700
https://hg.mozilla.org/mozilla-central/rev/554f19622867
https://hg.mozilla.org/mozilla-central/rev/8b5a47613fe6