1223586 - Transparent BrowserSearch appears when pasting into urlbar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

* Long press url bar
* Select paste

Expected: BrowserSearch appears
Actual: BrowserSearch appears, you can see the list items but you can see through to the page content

This is because BrowserSearch is transparent and adds back the window background in order to keep the search engine bar above the keyboard.

If the window background is already added by that time, we could be sure to hide the web content sooner. If not, we could try adding a background to BrowserSearch temporarily and remove it once the window background is shown (there will be some temporary overdraw) or try to add the background and hide the web content sooner.

Comment 1

[Alex Johnson (:alex_johnson)]

I would like to work on this one.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Alex Johnson(:alex_johnson) from comment #1)
> I would like to work on this one.

Sure! Let me know if you need any guidance.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot of the issue - GS4

12:34 <alex_johnson> mcomella: Was the BrowserSearch just temporarily trasparent for you or did it stay transparent even after the list was rendered?

BrowserSearch is temporarily transparent. This doesn't happen the first time (I assume because BrowserSearch has yet to load).

Comment 4

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

Bug 1223586 - Changed background and hid web content before showing browser search. r?mcomella

Comment 5

[Alex Johnson (:alex_johnson)]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26149/diff/1-2/

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

https://reviewboard.mozilla.org/r/26149/#review24105

::: mobile/android/base/BrowserApp.java:2556
(Diff revision 2)
> -        mBrowserSearchContainer.setVisibility(View.VISIBLE);
> +        if(mLayerView.getVisibility() == View.VISIBLE) {

nit: `if (...`

::: mobile/android/base/BrowserApp.java:2557
(Diff revision 2)
> +            mLayerView.setVisibility(View.INVISIBLE);

Why do you check if the LayerView is visible before hiding it? Can't you just hide it?

You can use the `BrowserApp.hideWebContent` method to do this (as we do when showing the HomePager).

Also, add a comment explaining why this is necessary.

::: mobile/android/base/BrowserApp.java:2602
(Diff revision 2)
> +        if(mLayerView.getVisibility() == View.INVISIBLE) {

nit `if (...`

::: mobile/android/base/BrowserApp.java:2603
(Diff revision 2)
> +            mLayerView.setVisibility(View.VISIBLE);

Hiding here seems unnecessary and can cause unnecessary overdraw in the following case:
1) Paste into the url bar
2) Delete the typed text

You can't see it because the `HomePager` draws over the page content but if you use the "Debug GPU overdraw" command, you'll see that it adds a layer of overdraw I'd like to avoid.

Why do you hide it here?

Sorry for the delay – I should have done this before the holiday!

Looking pretty close – just a few questions and I think we'll be good to go!

Comment 7

[Alex Johnson (:alex_johnson)]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26149/diff/2-3/

Comment 8

[Alex Johnson (:alex_johnson)]

https://reviewboard.mozilla.org/r/26149/#review24105

> Hiding here seems unnecessary and can cause unnecessary overdraw in the following case:
> 1) Paste into the url bar
> 2) Delete the typed text
> 
> You can't see it because the `HomePager` draws over the page content but if you use the "Debug GPU overdraw" command, you'll see that it adds a layer of overdraw I'd like to avoid.
> 
> Why do you hide it here?

I was making the web content visible before hiding the BrowserSearch.  The reviewboard shows this in the showBrowserSearch() function for some reason.  This is actually in the hideBrowserSearch() function.

Comment 9

[Alex Johnson (:alex_johnson)]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26149/diff/3-4/

Comment 10

[Alex Johnson (:alex_johnson)]

(In reply to Alex Johnson(:alex_johnson) from comment #9)
> Comment on attachment 8691578 [details]
> MozReview Request: Bug 1223586 - Hid web content and the home pager before
> making browser search visible. r?mcomella
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/26149/diff/3-4/

I rebased this commit on top of Bug 1107811 to avoid merge conflicts.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Alex Johnson(:alex_johnson) from comment #10)
> I rebased this commit on top of Bug 1107811 to avoid merge conflicts.

Great! Thanks for being proactive!

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/26149/#review24105

> I was making the web content visible before hiding the BrowserSearch.  The reviewboard shows this in the showBrowserSearch() function for some reason.  This is actually in the hideBrowserSearch() function.

I saw this in `hideBrowserSearch` before (maybe you just needed to expand the code?). I'm pretty sure we never go directly from BrowserSearch -> web content without going through "editing mode" (which shows web content itself) so this is unnecessary (and causes the overdraw problems above). What do you think?

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sorry for the delay – the team is at a conference this week so I've been intermittently available. I'll do my best to keep responding to you quickly, however.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

https://reviewboard.mozilla.org/r/26149/#review24835

Comment 15

[Alex Johnson (:alex_johnson)]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26149/diff/4-5/

Comment 16

[Alex Johnson (:alex_johnson)]

https://reviewboard.mozilla.org/r/26149/#review24105

> I saw this in `hideBrowserSearch` before (maybe you just needed to expand the code?). I'm pretty sure we never go directly from BrowserSearch -> web content without going through "editing mode" (which shows web content itself) so this is unnecessary (and causes the overdraw problems above). What do you think?

Gotcha!  All fixed!  :)

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

https://reviewboard.mozilla.org/r/26149/#review25273

Just one more question and I think we're good!

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2633
(Diff revision 5)
> +        //Show the BrowserSearch Container after the search list has been loaded

Why is this move necessary? afaik, the visibility calls only set some flags that the system uses on the next free UI thread cycle (i.e. after this method returns) to change the visibility. End result: any `setVisibility` calls you make here get batched on the next UI thread call.

Comment 18

[Alex Johnson (:alex_johnson)]

Attachment: rb26149.patch

I did not realize that just a flag was set and the system actually carried out the action during the next thread cycle.  That makes sense!  Thanks!  :)

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8699331 [details] [diff] [review]
rb26149.patch

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

Great! Works for me, thanks Alex!

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e75bd83f2e38

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Alex, I'm having trouble applying this patch to send it to our test servers – do you mind rebasing and making the push before adding "checkin-needed"?

Comment 22

[Alex Johnson (:alex_johnson)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=248f532c6c8c

Comment 23

[Alex Johnson (:alex_johnson)]

All set!  :)

Comment 24

[Carsten Book [:Tomcat]]

this failed to apply:

Parsing... done
adding 1223586 to series file
renamed 1223586 -> rb26149.patch
applying rb26149.patch
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh rb26149.patch

Comment 25

[Alex Johnson (:alex_johnson)]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26149/diff/5-6/

Comment 26

[Alex Johnson (:alex_johnson)]

That patch was corrupt somehow.  So, I went back to using reviewboard.  mcomella, could you check if you're able to import this?

Comment 27

[Alex Johnson (:alex_johnson)]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26149/diff/6-7/

Comment 28

[Alex Johnson (:alex_johnson)]

Rebased on top of the latest build incase that might have broken something.

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Alex Johnson (:alex_johnson) from comment #26)
> That patch was corrupt somehow.  So, I went back to using reviewboard. 
> mcomella, could you check if you're able to import this?

I was able to import via reviewboard and rebase on the latest fx-team.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8691578 [details]
MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella

https://reviewboard.mozilla.org/r/26149/#review25599

You already got an r+ so need to ask for another one! :)

Let me know if you still have trouble checking in.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e2e9a6c0f0f

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e2e9a6c0f0f