844816 - Awesome-screen tab text becomes indiscernible after font color changes from opening a private tab via context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul Feher]

Aurora 21.0a2 2012-02-24
Device: Samsung Galaxy R (Android 2.3.4) , LG Slider ARMv6 (Android 2.3.4) 

Steps to reproduce:
1. Open awesome screen.
2. Long tap on any link listen in Top Sites for the context menu tu appear.
3. Select open in private tab option.
4. Click on History section.

Expected results:
The awesome screen sections should keep the same font colour.

Actual results:
The awesome screen sections become white after opening a private tab from context menu. They are impossible to be read because the background remains also white.

Comment 1

[Aaron Train [:aaronmt]]

Is Fx20 affected?

Comment 2

[Aaron Train [:aaronmt]]

Yeah reproducible on my Nexus 4

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Paul would you retest with a build that has the fix for 845572 and see if this still happens?

Comment 4

[Adrian Tamas (:AdrianT)]

This is also an issue on Firefox 20 beta 2 build 3 if you open a normal tab from the context menu in the private tab awesomesceen on the Acer A500 (Android 3.2.1)

Comment 5

[Paul Feher]

Yes this is still reproducible on latest Nightly 22.0a1 2012-03-01 using the same device.

Comment 6

[Aaron Train [:aaronmt]]

Attachment: Nightly (03/14) - Screenshot

Screenshot.

Still reproducible on trunk and all channels.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: Remove menu items

Menu items are removed from the XML file. (Strings as a separate patch).

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Remove strings

This patch removes the strings (release-cycle friendly :D ).

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 727875 [details] [diff] [review]
Part 1: Remove menu items

This can only land if we also add these action to the about:home thumbnails

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 727877 [details] [diff] [review]
Part 2: Remove strings

We'll need the strings for the about:home actions

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3: about:home

Moved the menu items to about:home.
1. The way the AboutHomeContent handled ContextMenu was wrong. The selected position is available in AdapterView.AdapterContextMenuInfo by default. That (could) should be used as its free. Using an mSelected to store and use it is unnecessary. This caused some cleanup in the code.
2. Added a LOADURL_BACKGROUND flag. I thought of adding a LOADURL_SELECTED flag. But then, this is the default case and none would be passing this. Hence, the exception background case has a flag (just like delay load).
3. A small change in browser.js as the way we processed "selected" was incomplete.

Comment 12

[Sriram Ramasubramanian [:sriram]]

Accidentally added:
//selectTab(added.getId());

I could remove this (or the entire block, as the browser.js handles it anyways).

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 729620 [details] [diff] [review]
Part 3: about:home

>diff --git a/mobile/android/base/Tabs.java b/mobile/android/base/Tabs.java

>+            args.put("selected", !background);

I am worried about changing this now. We could file a new bug to handle this, if we even want to do it.

>-        if ((added != null) && !delayLoad) {
>-            selectTab(added.getId());
>+        if ((added != null) && !delayLoad && !background) {
>+            //selectTab(added.getId());

Keep the "selectTab" in place for now. We need it for time when Gecko (JS) is not ready yet... primarily during startup.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>         let params = {
>-          selected: !delayLoad,
>+          selected: ("selected" in data) ? data.selected : !delayLoad,

Let's move this to a new bug (see above) if we want it at all

r+, but revert the parts I called out.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 729620 [details] [diff] [review]
> Part 3: about:home
> 
> >diff --git a/mobile/android/base/Tabs.java b/mobile/android/base/Tabs.java
> 
> >+            args.put("selected", !background);
> 
> I am worried about changing this now. We could file a new bug to handle
> this, if we even want to do it.
> 
> >-        if ((added != null) && !delayLoad) {
> >-            selectTab(added.getId());
> >+        if ((added != null) && !delayLoad && !background) {
> >+            //selectTab(added.getId());
> 
> Keep the "selectTab" in place for now. We need it for time when Gecko (JS)
> is not ready yet... primarily during startup.
> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >         let params = {
> >-          selected: !delayLoad,
> >+          selected: ("selected" in data) ? data.selected : !delayLoad,
> 
> Let's move this to a new bug (see above) if we want it at all
> 
> r+, but revert the parts I called out.

Talked to Brian and Sriram on IRC. It seems we have no way to open a tab in the background from Java. We'll need _all_ of this patch. Make it so!

Comment 15

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc00926f486
https://hg.mozilla.org/integration/mozilla-inbound/rev/046ff5c0a0ba

Comment 16

[Wesley Johnston (:wesj)]

backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c28f3dfd9a0e

Comment 17

[Sriram Ramasubramanian [:sriram]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9ce01b2e6091

Comment 18

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Remove tests in AwesomeBar

This doesn't search for the "Open in new tab" menu item.

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 730970 [details] [diff] [review]
Patch: Remove tests in AwesomeBar

Looks good to me, but why abort the try build?

Comment 20

[Sriram Ramasubramanian [:sriram]]

Try with new changes: https://tbpl.mozilla.org/?tree=Try&rev=c6820073dfce

Comment 21

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Remove tests in AwesomeBar

This removes everywhere.

Comment 22

[Sriram Ramasubramanian [:sriram]]

Pushed with tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c710854a413f
https://hg.mozilla.org/integration/mozilla-inbound/rev/36eca1fb1c5c

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c710854a413f
https://hg.mozilla.org/mozilla-central/rev/36eca1fb1c5c

Comment 24

[Adrian Tamas (:AdrianT)]

Verified fix on Firefox Mobile 22 beta 3 on the Asus EEE Transformer TF101 (Android 4.0.4). Removing the options from the context menu prevents the user from hitting the state described in Comment 0.