630946 - Repaint does not occur when dismissing a combo box

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Android; Linux armv71; rv2.0b11pre) Gecko/20110202 Firefox/4.0b11pre Fennec/4.0b5pre

1. go to people.mozilla.com/~nhirata/html_tp/formsninput.html
2. scroll down to the checkbox
3. select the checkbox
4. click about the selections to cancel out

Expected:  Repaint occurs
Actual: repaint does not occur, panning will repaint

Note:
1. Android Only

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: Screenshot

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

oops.  I meant combo box.  Corrected title.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

I think we need to make sure the displayport is updated. I think we could use a version of the "resize" patch I have in a different bug to make this more robust. I don't want to put a "fix" in many places.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

There is 2 differents bug here:
 * firstly there is some code path where finishFuzzyZoom() is never called
 * Secondly, when the browser is scrolled to the max and the size changed the fuzzyZoom method does not ensure the content is not overscrolled

Comment 5

[Benjamin Stover (:stechz)]

Comment on attachment 510297 [details] [diff] [review]
Patch

I had hoped fuzzyZoom could be an API that does not care about the viewport or content, but simply shows the rectangle requested. If the zoom code does not know exactly what rectangle we're zooming to (it should already have clipped), then the zoom will look weird at the end.

Could you explain what happens if this code isn't present? If it is AnimatedZoom's fault, AnimatedZoom needs to take the size and position of the browser into account when determining the final zoom rectangle.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #5)
> Comment on attachment 510297 [details] [diff] [review]
> Patch
> 
> I had hoped fuzzyZoom could be an API that does not care about the viewport or
> content, but simply shows the rectangle requested. If the zoom code does not
> know exactly what rectangle we're zooming to (it should already have clipped),
> then the zoom will look weird at the end.
> 

If needed I could move the code out fuzzyZoom, I won't mind adding it inside the ViewableAreaObserver.update method

> Could you explain what happens if this code isn't present? 

let me add a screenshot in a sec, it will be easier to understand

> AnimatedZoom's fault, AnimatedZoom needs to take the size and position of the
> browser into account when determining the final zoom rectangle.

This is a code path where AnimatedZoom is not used, so it is not guilty here :)

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot

At the bottom of the screenshot there is some permanent checkerboarding, this is because the restoreViewportPosition try to scroll to the same position as before the size changed.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Move the code out of fuzzyZoom

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Looking at bug 630736 I've found an other step to reproduce that does not involve the Form Assistant.

Steps to reproduce:
 * go to liliputing.com in a landscape view
 * zoom in
 * scroll to the end of the page
 * hit Ctrl+Shift+q to switch orientation

Actual result: 
 * The bottom of the page is only checkerboarding

Expected result:
 * no checkerboarding

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 510567 [details] [diff] [review]
Patch v0.3


>diff --git a/chrome/content/common-ui.js b/chrome/content/common-ui.js

>   set _open(aVal) {
>     if (aVal == this._open)
>       return;
> 
>     this._container.hidden = !aVal;
>-    this._container.contentHasChanged();
> 
>     if (aVal) {
>+      this._container.contentHasChanged();
>       this._zoomStart();

This change seems strange to me. Why do we only notify when opening? Wouldn't closing also seem to change the size?

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #11)
> Comment on attachment 510567 [details] [diff] [review]
> Patch v0.3
> 
> 
> >diff --git a/chrome/content/common-ui.js b/chrome/content/common-ui.js
> 
> >   set _open(aVal) {
> >     if (aVal == this._open)
> >       return;
> > 
> >     this._container.hidden = !aVal;
> >-    this._container.contentHasChanged();
> > 
> >     if (aVal) {
> >+      this._container.contentHasChanged();
> >       this._zoomStart();
> 
> This change seems strange to me. Why do we only notify when opening? Wouldn't
> closing also seem to change the size?

It does, this change is not needed for the patch to works, but since the this._container.hide method is going to call contentHasChanged there is no need to duplicate the work

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #12)

> It does, this change is not needed for the patch to works, but since the
> this._container.hide method is going to call contentHasChanged there is no need
> to duplicate the work

Hmm. Seems like contentHasChanged() is called from container.show() too. Let's try to remove contentHasChanged() from _open altogether.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.4

let's try that.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 510614 [details] [diff] [review]
Patch v0.4

r+ and you should do some manual + browser-chrome tests to make sure nothing seems to be broken

Comment 16

[Benjamin Stover (:stechz)]

Comment on attachment 510614 [details] [diff] [review]
Patch v0.4

Thanks for your work on this! :)

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/953124a5dc82

Comment 18

[Aaron Train [:aaronmt]]

Verified fixed with Mozilla/5.0 (Android; Linux armv7l; rv:2.0b12pre) Gecko/20110215
Firefox/4.0b12pre Fennec/4.0b5pre