Transparent BrowserSearch appears when pasting into urlbar

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: alex_johnson, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 46
All
Android
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [lang=java])

Attachments

(2 attachments, 1 obsolete attachment)

* 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.
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Created attachment 8691569 [details]
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).
(Assignee)

Comment 4

3 years ago
Created attachment 8691578 [details]
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
Attachment #8691578 - Flags: review?(michael.l.comella)
(Assignee)

Comment 5

3 years ago
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!
(Assignee)

Comment 7

3 years ago
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)
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
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/
(Assignee)

Comment 10

3 years ago
(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)
(Assignee)

Updated

3 years ago
Attachment #8691578 - Flags: review?(michael.l.comella)
(Assignee)

Comment 15

3 years ago
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/
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8691578 - Attachment is obsolete: true
(Assignee)

Comment 18

3 years ago
Created attachment 8699331 [details] [diff] [review]
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!  :)
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)
(Assignee)

Comment 23

3 years ago
All set!  :)
Flags: needinfo?(me)
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 25

3 years ago
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)
(Assignee)

Updated

3 years ago
Attachment #8699331 - Attachment is obsolete: true
Flags: needinfo?(me)
(Assignee)

Comment 26

3 years ago
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)
(Assignee)

Comment 27

3 years ago
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/
(Assignee)

Comment 28

3 years ago
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+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 32

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e2e9a6c0f0f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.