Closed Bug 990974 Opened 7 years ago Closed 6 years ago

Unexpected checkerboarding when scrolling input field in Contacts app

Categories

(Core :: Layout, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: botond, Assigned: mstange)

References

Details

Attachments

(10 files, 1 obsolete file)

429 bytes, text/html
Details
6.40 KB, text/plain
Details
685 bytes, patch
Details | Diff | Splinter Review
1.18 KB, patch
tnikkel
: feedback+
Details | Diff | Splinter Review
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
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.
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.
Does this still happen?
Flags: needinfo?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Does this still happen?

Just checked with a recent trunk. It still happens.
Flags: needinfo?(botond)
tn, do you know if input fields also use nsGfxScrollFrame?
Flags: needinfo?(tnikkel)
Yes, I think they do.
Flags: needinfo?(tnikkel)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Attached file Standalone test case
The div scrolls without checkerboarding, but the input element has checkerboarding. They should really behave the same.
Attachment #8473429 - Attachment mime type: text/plain → text/html
Attached file Layer dump for test case (obsolete) —
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.
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.
Attachment #8473458 - Attachment is obsolete: true
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.
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?
Flags: needinfo?(tnikkel)
Bug 965237 added this earlier this year. CC'ing mats also but he's on vacation.
Blocks: 965237
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.
Flags: needinfo?(tnikkel)
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)
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.
Component: Panning and Zooming → Layout
I can mentor, patch, or review. :)
This isn't particularly high priority so let's go with mentoring :)
Mentor: tnikkel
Assignee: nobody → nicklebedev37
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.
Flags: needinfo?(nicklebedev37)
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).
Flags: needinfo?(nicklebedev37)
tn ^
Flags: needinfo?(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?
Flags: needinfo?(tnikkel)
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.
Attachment #8511990 - Flags: feedback?(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.
Attachment #8511990 - Flags: feedback?(tnikkel) → feedback+
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?
Flags: needinfo?(mats)
> 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.)
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
> 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?
Flags: needinfo?(mats) → needinfo?(roc)
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.
Flags: needinfo?(roc)
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.
Depends on: 866731
(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.
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.
Assignee: nicklebedev37 → nobody
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Bug 990974 - Use variadic templates for ClipListsExceptCaret.
Attachment #8629052 - Flags: review?(roc)
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.
Attachment #8629053 - Flags: review?(roc)
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.
Attachment #8629054 - Flags: review?(roc)
Bug 990974 - Add a flag to ComputeFrameMetrics so that a different clip can be returned for the caret and non-caret content.
Attachment #8629055 - Flags: review?(roc)
Bug 990974 - Give PaintedLayerDataTree a generic way of restricting a PaintedLayer to exactly one display item.
Attachment #8629056 - Flags: review?(roc)
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.
Attachment #8629057 - Flags: review?(roc)
Blocks: 1178745
Comment on attachment 8629052 [details]
MozReview Request: Bug 990974 - Use variadic templates for ClipListsExceptCaret.

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

Ship It!
Attachment #8629052 - Flags: review?(roc) → review+
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!
Attachment #8629054 - Flags: review?(roc) → review+
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 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 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!
(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.
You need to log in before you can comment on or make changes to this bug.