Closed Bug 630946 Opened 9 years ago Closed 9 years ago

Repaint does not occur when dismissing a combo box

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
All
defect
Not set

Tracking

(fennec2.0b5+)

VERIFIED FIXED
Tracking Status
fennec 2.0b5+ ---

People

(Reporter: nhirata, Assigned: vingtetun)

References

()

Details

Attachments

(3 files, 3 obsolete files)

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
oops.  I meant combo box.  Corrected title.
Summary: Repaint does not occur when dismissing a checkbox list → Repaint does not occur when dismissing a combo box
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.
Assignee: nobody → 21
tracking-fennec: --- → 2.0b5+
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #510297 - Flags: review?(mark.finkle)
Attachment #510297 - Flags: review?(ben)
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.
Attachment #510297 - Flags: review?(ben)
(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 :)
Attached image 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.
Attached patch Patch v0.2 (obsolete) — Splinter Review
Move the code out of fuzzyZoom
Attachment #510297 - Attachment is obsolete: true
Attachment #510309 - Flags: review?(mark.finkle)
Attachment #510309 - Flags: review?(ben)
Attachment #510297 - Flags: review?(mark.finkle)
Attached patch Patch v0.3 (obsolete) — Splinter Review
Attachment #510309 - Attachment is obsolete: true
Attachment #510567 - Flags: review?(mark.finkle)
Attachment #510567 - Flags: review?(ben)
Attachment #510309 - Flags: review?(mark.finkle)
Attachment #510309 - Flags: review?(ben)
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 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?
(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
(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.
Attached patch Patch v0.4Splinter Review
let's try that.
Attachment #510567 - Attachment is obsolete: true
Attachment #510614 - Flags: review?(mark.finkle)
Attachment #510614 - Flags: review?(ben)
Attachment #510567 - Flags: review?(mark.finkle)
Attachment #510567 - Flags: review?(ben)
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
Attachment #510614 - Flags: review?(mark.finkle) → review+
Comment on attachment 510614 [details] [diff] [review]
Patch v0.4

Thanks for your work on this! :)
Attachment #510614 - Flags: review?(ben) → review+
http://hg.mozilla.org/mobile-browser/rev/953124a5dc82
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Verified fixed with Mozilla/5.0 (Android; Linux armv7l; rv:2.0b12pre) Gecko/20110215
Firefox/4.0b12pre Fennec/4.0b5pre
Status: RESOLVED → VERIFIED
OS: Android → All
You need to log in before you can comment on or make changes to this bug.