raymond [:retornam] (inactive) Reporter
	Description • 11 years ago

STR:
Using  http://hg.mozilla.org/mozilla-central/rev/02aa81c59df6
1) Visit https://www.google.com
2) Swipe from the bottom to bring up the context bar
3) Tap the settings icon to bring up a context menu and select the Find in page  item.
4) Type Google in the Find in page box
5) Click / Tap on the down icons to switch between matched search terms

Current Results:
1) The  Google+ and About Google texts at the bottom right hand corner of the page are matched but not highlighted because the context bar covers them


Expected Results:

All matched text should be shown

	Marco Mucci [:MarcoM]
	Comment 1 • 11 years ago

Hey Raymond, which Story is this defect related to?

	raymond [:retornam] (inactive) Reporter
	Comment 2 • 11 years ago

bug 831940  Story - Find previous and next instance of text in a page

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 3 • 11 years ago

Fixed it by adding some padding to the bottom of the browser. I also added a transition when showing/hiding the find bar. Now it behaves like desktop: the bar slides in and pushes the browser viewport up. On pages like google.com you can see the bottom part of the page sliding up. The page does not move if you can scroll down.

I ended up adding a function to browser.js to add the padding dynamically. My first thought was to add an attribute or class to #browsers to keep the view decoupled, but I liked being able to specify the height dynamically. I think it can be reused by other bars (add-on bar !? :) ). I can change it back if you prefer, all bars should have the same height anyway.

I considered refactoring ContentAreaObserver._shiftBrowserDeck to make it less keyboard specific, but using it hides the top of the page, which means we'd have to only do it when the highlight is covered by the find bar, which differs from what we do on desktop.

	Matt Brubeck (:mbrubeck)
	Comment 4 • 11 years ago

Comment on attachment 752775 [details] [diff] [review]
Patch v1

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

I'd like to try out some possible changes before we check this in:

::: browser/metro/base/content/browser.js
@@ +477,5 @@
>    },
>  
> +  padBottom: function padBottom(height) {
> +    this.selectedBrowser.style.paddingBottom = height + "px";
> +  },

This is a little fragile since two modules trying to use it at the same time will tend to overwrite each other's changes.  Maybe we should make the method add and subtract padding instead.

Instead of selectedBrowser, can we resize all the browsers (or maybe the whole "page" vbox)?  Perhaps we could even put the content-navigator inside of <vbox id="page"> and just modify its height; the browser deck should then resize automatically without any code here.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 5 • 11 years ago

WIP 

I ended up moving #content-navigator inside #page as you suggested. It's much cleaner. The browser deck had fixed width, I updated it to flex and made #page have the fixed width properties.

I'm using margin-bottom for the transform because height was distorting the images even with overflow hidden.

I'm marking it as WIP because repositioning on OSK when an input is selected broke with this change.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 6 • 11 years ago

The OSK repositioning issue is due to the fact that #browsers will now flex to the bottom of the screen after changing the negative margin on ContentAreaObserver._shiftBrowserDeck().

The simplest solution is to use translateY instead of a negative margin here https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/ContentAreaObserver.js#264

Jim, you added a comment that translate will cause us selectAtPoint related problems because of bug 858471. I tried selection while keyboard is up and it worked fine for me. What type of problems are expected because of bug 858471?

	Jim Mathies [:jimm]
	Comment 7 • 11 years ago

(In reply to Rodrigo Silveira [:rsilveira] from comment #6)
> The OSK repositioning issue is due to the fact that #browsers will now flex
> to the bottom of the screen after changing the negative margin on
> ContentAreaObserver._shiftBrowserDeck().
> 
> The simplest solution is to use translateY instead of a negative margin here
> https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> ContentAreaObserver.js#264
> 
> Jim, you added a comment that translate will cause us selectAtPoint related
> problems because of bug 858471. I tried selection while keyboard is up and
> it worked fine for me. What type of problems are expected because of bug
> 858471?

When I worked on this it wasn't, selectAtPoint was selecting where the browser was without the transform. Although I ran into this again in working on chrome selection, worked up a simple test case, and couple reproduce on the app/address bar combo which uses a transform. So maybe the bug is somewhere else, or got fixed.

If you want to change this to a transform I'd suggest also working up a selection test that calls shiftBrowserDeck and attempts to select text in an input and content. (Making sure the coordinates you choose match where text *should* be selected will be important, it's easy to choose random coordinate points and assume the text that is select is at the right place.)

let promise = waitForEvent(window, "MozDeckOffsetChanged");
ContentAreaObserver._shiftBrowserDeck(300);
yield promise;
(select)

	Jim Mathies [:jimm]
	Comment 8 • 11 years ago

* and couple reproduce -> and couldn't reproduce

	Jim Mathies [:jimm]
	Comment 9 • 11 years ago

Note we already have a selection test that runs when the notification bar comes down which works fine. I think this is because the notification bar takes up browser view space. I'm not sure what we are trying to do with the find bar here but I'd guess the same type of shifting could be used, only from the bottom up.

	Matt Brubeck (:mbrubeck)
	Comment 10 • 11 years ago

Comment on attachment 753057 [details] [diff] [review]
Patch v2 WIP

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

Looking good!

::: browser/metro/theme/browser.css
@@ +734,5 @@
>  }
>  
> +/* Make sure the browser takes the entire space */
> +browser {
> +  -moz-box-flex: 1;

Instead of setting defaults for all <browser> elements, let's limit this to "#browsers browser" or something equivalent.

@@ +735,5 @@
>  
> +/* Make sure the browser takes the entire space */
> +browser {
> +  -moz-box-flex: 1;
> +  -moz-box-orient: vertical;

Are you sure this -moz-box-orient is needed?  I thought this only affected elements with children.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 11 • 11 years ago

OK, now I was able to repro the issue. The context menu select option fails with the translate :(. It's using selectAtPoint eventually. 

It worked when I call Browser.ptBrowserToClient() before sending the "Browser:SelectionStart" msg, here https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/helperui/SelectionHelperUI.js#390

But selectionAtPoint expects browser position, not chrome window position, right? And the monocle is not showing if you have the browser deck shifted with this hack.

I'm inclined to either go back to using the negative margin again or adding a spacer inside #page to shift the browser up.

(In reply to Jim Mathies [:jimm] from comment #9)
> Note we already have a selection test that runs when the notification bar
> comes down which works fine. I think this is because the notification bar
> takes up browser view space. I'm not sure what we are trying to do with the
> find bar here but I'd guess the same type of shifting could be used, only
> from the bottom up.

I wasn't able to find a selection test with notification bar, only a context menu test.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 12 • 11 years ago

OK, I am getting too deep in the rabbit hole :) I think it's too risky to mess up with selection for this bug. And I'll probably be in the way of bug 857437.

I pretty much reverted back to the first patch and now #content-navigator is back outside #page. I'm using an attribute to add the padding to all browsers when the find bar is shown, instead of a pseudo-generic method.

I like the idea of having the find bar and a spacer for keyboard inside #page and eliminating the translate, but it's more involved. We could have a separate bug on that if you think it's useful.

	Jim Mathies [:jimm]
	Comment 13 • 11 years ago

(In reply to Rodrigo Silveira [:rsilveira] from comment #11)
> OK, now I was able to repro the issue. The context menu select option fails
> with the translate :(. It's using selectAtPoint eventually. 
> 
> It worked when I call Browser.ptBrowserToClient() before sending the
> "Browser:SelectionStart" msg, here
> https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> helperui/SelectionHelperUI.js#390
> 
> But selectionAtPoint expects browser position, not chrome window position,
> right?

Looks like openEditSession takes browser relative coordinates and doesn't translate for chrome offsets. Maybe that's the problem here, I find callers - 

https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/BrowserTouchHandler.js#30
https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/ContextCommands.js#103

One is sending what appears to be context menu position coordinates which are screen coords - 

https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/helperui/MenuUI.js#125
https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/helperui/MenuUI.js#195

the other in BrowserTouchHandler is sending browser relative coords which come over from the ContextMenuHandler.

> (In reply to Jim Mathies [:jimm] from comment #9)
> > Note we already have a selection test that runs when the notification bar
> > comes down which works fine. I think this is because the notification bar
> > takes up browser view space. I'm not sure what we are trying to do with the
> > find bar here but I'd guess the same type of shifting could be used, only
> > from the bottom up.
> 
> I wasn't able to find a selection test with notification bar, only a context
> menu test.

Ah yes, sorry, I had that context menu test confused with selection tests.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 14 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/482b8c3e91f7

	Ed Morley [:emorley]
	Comment 15 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/482b8c3e91f7

	Marco Mucci [:MarcoM]
	Comment 16 • 11 years ago

For testing and verification.

	raymond [:retornam] (inactive) Reporter
	Comment 17 • 11 years ago

verified fixed  

Nightly 24.0a1 (2013-06-04)

	Kamil Jozwiak [:kjozwiak]
	Comment 18 • 11 years ago

Went through the following "Defect" for iteration #9 testing with some issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-07-03-03-13-23-mozilla-central/

- Went through the original test case that has been added in comment 0 without any issues
- Ensured that the "Find Bar" pushes the website up if there's no scrolling as per comment 3
- Ensured that the "Find Bar" doesn't push the website up if there's scrolling as per comment 3
- Ensured that the website scrolls through the searched words with & without the OSK on scrollable websites 

The following issues where encountered when going through the following defect:

- Created Bug 890148
- Created Bug 890153

	Samvedana (:Samvedana)
	Comment 19 • 11 years ago

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130812030209
Built from http://hg.mozilla.org/mozilla-central/rev/87c1796bc46c

WFM
Tested on windows 8 using latest nightly for iteration-11. Followed steps provided in comment0 and got expected result.

	Samvedana (:Samvedana)
	Comment 20 • 11 years ago

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.