Closed Bug 1223586 Opened 6 years ago Closed 6 years ago

Transparent BrowserSearch appears when pasting into urlbar

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: mcomella, Assigned: alex_johnson, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(2 files, 1 obsolete file)

* 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.
I would like to work on this one.
(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.
Assignee: nobody → mrjohnsonalex
Status: NEW → ASSIGNED
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).
Bug 1223586 - Changed background and hid web content before showing browser search. r?mcomella
Attachment #8691578 - Flags: review?(michael.l.comella)
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/
Attachment #8691578 - Attachment description: MozReview Request: Bug 1223586 - Changed background and hid web content before showing browser search. r?mcomella → MozReview Request: Bug 1223586 - Hid web content and the home pager before making browser search visible. r?mcomella
Attachment #8691578 - Flags: review?(michael.l.comella)
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 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/
Attachment #8691578 - Flags: review?(michael.l.comella)
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 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/
(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.
(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!
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?
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 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
Attachment #8691578 - Flags: review?(michael.l.comella)
Attachment #8691578 - Flags: review?(michael.l.comella)
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/
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!  :)
Attachment #8691578 - Flags: review?(michael.l.comella)
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.
Attachment #8691578 - Attachment is obsolete: true
Attached patch rb26149.patch (obsolete) — Splinter Review
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!  :)
Attachment #8699331 - Flags: review?(michael.l.comella)
Comment on attachment 8699331 [details] [diff] [review]
rb26149.patch

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

Great! Works for me, thanks Alex!
Attachment #8699331 - Flags: review?(michael.l.comella) → review+
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"?
Flags: needinfo?(me)
All set!  :)
Flags: needinfo?(me)
Keywords: checkin-needed
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
Flags: needinfo?(me)
Keywords: checkin-needed
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/
Attachment #8691578 - Attachment is obsolete: false
Attachment #8691578 - Flags: review?(michael.l.comella)
Attachment #8699331 - Attachment is obsolete: true
Flags: needinfo?(me)
That patch was corrupt somehow.  So, I went back to using reviewboard.  mcomella, could you check if you're able to import this?
Flags: needinfo?(michael.l.comella)
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/
Rebased on top of the latest build incase that might have broken something.
(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.
Flags: needinfo?(michael.l.comella)
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.
Attachment #8691578 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e2e9a6c0f0f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.