Closed Bug 595233 Opened 9 years ago Closed 9 years ago

Fennec FormHelper does not zoom to the correct position on all platforms

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b2+)

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: vingtetun, Assigned: jbos)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Since the landing of bug 587492 (which i reviewed), the zooming code of the Form Helper does not zoom to the correct position on Maemo/Desktop/Android.
The current patch revert mostly to the previous behavior but with a custom path for Meego nested into the visibleScreenArea getter and the observer.

I would like to have a chance to test it on Meego but I don't have any way to do it. The way I've check the behavior is using a custom _visibleScreenArea property while zooming.
Attachment #474098 - Flags: review?(mark.finkle)
Attached patch Patch v0.2 (obsolete) — Splinter Review
Wrong patch, this one fix a little bug with the caret rect re-positionning.
Attachment #474098 - Attachment is obsolete: true
Attachment #474100 - Flags: review?(mark.finkle)
Attachment #474098 - Flags: review?(mark.finkle)
What do you mean with "wrong" position?

I wonder what is the correct position :). One issue seem to be that we hide the Awesomebar but its still visible. 

Another "misunderstanding" seem that the code always move the caret to the upper edge of the screen. Which is actually meant to be. 

For the case that it shouldn't be the upper edge, but centered, I gave mark a patch. :) Let us please not through the hard work in this piece of code away ;) It took me a lot of nerves to find the perfect meego solution.
To get it centered: Just do this 

       let x = (marginLeft + marginRight + margin + aCaretRect.x -  aElementRect.x) < viewAreaWidth
                ? aElementRect.x - margin - marginLeft
                : aCaretRect.x - viewAreaWidth + margin + marginRight;
       // use the adjustet Caret Y minus a margin four our visible rect
-      let y = harmonizedCaretY - margin;
+      let y = harmonizedCaretY - margin - viewAreaHeight / 3;

      // from here on play with zoomed values
      // if we want to have it animated, build up zoom rect and animate.
Vivien - Does Jeremias' patch improve the situation without doing a full backout?
Blocks: 584225
Sorry for the long time before responding, I was trying to take a decision about this patch and what we should do with it.

(In reply to comment #2)
> What do you mean with "wrong" position?
> > I wonder what is the correct position :). One issue seem to be that we hide the> Awesomebar but its still visible. 

In my opinion, the correct position is to have the current form's element centered both horizontally and vertically: 
  * first because this is the first place I look once the content has changed (but you can reply people are looking into a UI using a "Z" exploration's technic) 
  * but mostly because this give me much more context about this element.

And yes the awesome bar is another issue but not the main reason for why I want to "revert" some part of this code.

> Another "misunderstanding" seem that the code always move the caret to the
> upper edge of the screen. Which is actually meant to be. 
> > For the case that it shouldn't be the upper edge, but centered, I gave mark a> patch. :) Let us please not through the hard work in this piece of code away ;)

I have tried tweaking the line as you mentionned but this is still not sufficient, there is still a zoom error when the form helper encountered a list element. 
Also I like having the animated zoom  effect even when the zoom size doesn't change (this helps figuring where we the form helper moves you).
Layers has landed now so the animated zoom should be much faster for you.

> It took me a lot of nerves to find the perfect meego solution.

Sure! I can only understand you on this point :).
I do not want to drop all your work but I want a solution that does not broke the other platforms for b2. My patch should keep the compatibility with Meego (I have not used the preferences though) but it is completely untested on Meego.


So what I would like to see is a patch that keep the element centered, zoom at the correct zoom level, and use an animation when moving from one element to another even if the zoom level has not changed.
If you want to work on it I would like to see if we can keep the specific Meego code nested into one place as I've done in my patch (I don't mind if you take my patch and build on top of it, or do critical changes)

We can discuss on IRC if you want, I think I remember that you come back from holiday today?
Comment on attachment 474100 [details] [diff] [review]
Patch v0.2

Remove r? for now
Attachment #474100 - Flags: review?(mark.finkle)
I'm back in the office, i will take care about this.
Status: NEW → ASSIGNED
Assignee: nobody → jeremias.bosch
Summary: Fennec does not zoom to the correct position on all platforms → Fennec FormHelper does not zoom to the correct position on all platforms
(In reply to comment #7)
> I'm back in the office, i will take care about this.

Any news on this?

I think bug 603612 and bug 597036 can be related to this code.
I'm starting to think of re-basing my backout and use it
yep, i had some very busy weeks, current plan sees this as target for end of next week

for both issues one important thing is the still missing rubberband, in both cases the textfield positioning migt also be restricted by the browser.

anyway i will propose the patch for centering - and some sort of sliding rubberband  ui is also in my mind... there will be progress now on this
tracking-fennec: --- → 2.0b2+
bug 602707 might be related too
Blocks: 605119
Attached patch Patch v0.3Splinter Review
The patch reverts a bunch of the code but keep using visibleScreenArea instead of the browser's visibleRect to compute the zoom position.
Attachment #474100 - Attachment is obsolete: true
Attachment #486108 - Flags: review?(mark.finkle)
Changing this code made me thought we should probably have a global approach for the Meego VKB and move the visibleScreenArea property to the Browser object to have it compute the zoom on it instead of overidding method like what I have done here.

This is an other bug but I just want people to think about it.
Comment on attachment 486108 [details] [diff] [review]
Patch v0.3


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

>+  _visibleScreenArea: null,
>+  get visibleScreenArea() {
>+    let visibleRect = Rect.fromRect(Browser.selectedBrowser.getBoundingClientRect());
>+    let visibleScreenArea = visibleRect;
>+    if (this._visibleScreenArea) {
>+     visibleScreenArea = this._visibleScreenArea.clone();
>+      visibleScreenArea.x = visibleRect.x;
>+      visibleScreenArea.y = visibleRect.y;
>+      visibleScreenArea.width = visibleRect.width;
>+      visibleScreenArea.height = visibleRect.height - this._container.getBoundingClientRect().height;

1 space indent problem

Looks ok. I think we takes this for b2 and start testing as many form / login sites as possible
Attachment #486108 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/f3c39ab699ec
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
We're seeing a number of breakages (two reported so far) in today's nightly builds that might be related to this bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=607634
https://bugzilla.mozilla.org/show_bug.cgi?id=607574
Flags: in-testsuite?
(In reply to comment #15)
> We're seeing a number of breakages (two reported so far) in today's nightly
> builds that might be related to this bug:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=607634
> https://bugzilla.mozilla.org/show_bug.cgi?id=607574

Bug 607574?
This is totaly broken on meego now. We scroll and zoom to wrong position again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #17)
> This is totaly broken on meego now. We scroll and zoom to wrong position again.

Lets open a new bug
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Blocks: 608368
Done, see Bug 608368
This issue doesn't occur anymore on the latest Nightly build (Android). Since a new bug was filled for MeeGo, I will mark this one as verified fixed.

--
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110913
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.