940632 - Fixup findbar scroll positioning code and remove some text zoom cruft

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

Improving how we position selected text generated by the find bar. This was partially broken, the css wasn't working and positioning was off.

Note I specifically do not want to run these coords through the browser translation routines in bindings since I'm controlling browser offsets here myself.

Also, this doesn't fix find text positioning with zoom. I've kicked off an email thread with kats and botond on this. We currently don't have a way to tell the apzc to center on a dom point which we'll need to make this work perfectly.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #2)
> Also, this doesn't fix find text positioning with zoom. I've kicked off an
> email thread with kats and botond on this. We currently don't have a way to
> tell the apzc to center on a dom point which we'll need to make this work
> perfectly.

There's a patch posted in bug 940952 for this. I had to touch up some of the scroll values using the current scale.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8335228 [details] [diff] [review]
patch v.1

Review of attachment 8335228 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/browser-scripts.js
@@ -101,5 @@
>    ["SelectHelperUI", "chrome://browser/content/helperui/SelectHelperUI.js"],
>    ["SelectionHelperUI", "chrome://browser/content/helperui/SelectionHelperUI.js"],
>    ["SelectionPrototype", "chrome://browser/content/library/SelectionPrototype.js"],
>    ["ChromeSelectionHandler", "chrome://browser/content/helperui/ChromeSelectionHandler.js"],
> -  ["AnimatedZoom", "chrome://browser/content/AnimatedZoom.js"],

Can we delete this file now?

::: browser/metro/base/content/helperui/FindHelperUI.js
@@ +193,4 @@
>  
> +    // Adjust for keyboad display and position the text selection rect in
> +    // the middle of the viewable area.
> +    let xPos = aElementRect.center().x;

Why are we scrolling to the horizontal center of the rect?  Shouldn't we scroll to the top left of the rect, so that its top left ends up near the top left of the screen?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> Comment on attachment 8335228 [details] [diff] [review]
> patch v.1
> 
> Review of attachment 8335228 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/browser-scripts.js
> @@ -101,5 @@
> >    ["SelectHelperUI", "chrome://browser/content/helperui/SelectHelperUI.js"],
> >    ["SelectionHelperUI", "chrome://browser/content/helperui/SelectionHelperUI.js"],
> >    ["SelectionPrototype", "chrome://browser/content/library/SelectionPrototype.js"],
> >    ["ChromeSelectionHandler", "chrome://browser/content/helperui/ChromeSelectionHandler.js"],
> > -  ["AnimatedZoom", "chrome://browser/content/AnimatedZoom.js"],
> 
> Can we delete this file now?

yeah totally.

> ::: browser/metro/base/content/helperui/FindHelperUI.js
> @@ +193,4 @@
> >  
> > +    // Adjust for keyboad display and position the text selection rect in
> > +    // the middle of the viewable area.
> > +    let xPos = aElementRect.center().x;
> 
> Why are we scrolling to the horizontal center of the rect?  Shouldn't we
> scroll to the top left of the rect, so that its top left ends up near the
> top left of the screen?

I'd also like to make this change. Since you're cool with this then I'll go ahead and change it. I filed bug 941032 on improving the logic here as well.

Comment 6

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/930f4fee3e3d

Comment 7

[Jim Mathies [:jimm]]

follow up to fix some test failures - 

https://hg.mozilla.org/integration/fx-team/rev/a15e63b297e4

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/930f4fee3e3d
https://hg.mozilla.org/mozilla-central/rev/a15e63b297e4