Closed Bug 844816 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

21 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox20 affected, firefox21 affected, firefox22 verified, fennec21+)

VERIFIED FIXED
Firefox 22
Tracking Status
firefox20 --- affected
firefox21 --- affected
firefox22 --- verified
fennec 21+ ---

People

(Reporter: paul.feher, Assigned: sriram)

References

Details

(Keywords: reproducible)

Attachments

(5 files, 1 obsolete file)

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.
Is Fx20 affected?
tracking-fennec: --- → ?
Keywords: reproducible
Summary: Awesome screen sections become white after opening a private tab from context menu → Awesome-screen tab text becomes discernable after font color changes from opening a private tab via context menu
Summary: Awesome-screen tab text becomes discernable after font color changes from opening a private tab via context menu → Awesome-screen tab text becomes indiscernible after font color changes from opening a private tab via context menu
Yeah reproducible on my Nexus 4
Depends on: 845572
Paul would you retest with a build that has the fix for 845572 and see if this still happens?
Assignee: nobody → sriram
tracking-fennec: ? → 20+
Flags: needinfo?(paul.feher)
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)
Yes this is still reproducible on latest Nightly 22.0a1 2012-03-01 using the same device.
Flags: needinfo?(paul.feher)
Flags: needinfo?(sriram)
Screenshot.

Still reproducible on trunk and all channels.
Status: NEW → ASSIGNED
tracking-fennec: 20+ → 21+
Menu items are removed from the XML file. (Strings as a separate patch).
Attachment #727875 - Flags: review?(mark.finkle)
Flags: needinfo?(sriram)
This patch removes the strings (release-cycle friendly :D ).
Attachment #727877 - Flags: review?(mark.finkle)
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
Attachment #727875 - Flags: review?(mark.finkle) → review+
Comment on attachment 727877 [details] [diff] [review]
Part 2: Remove strings

We'll need the strings for the about:home actions
Attachment #727877 - Flags: review?(mark.finkle) → review-
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.
Attachment #729620 - Flags: review?(mark.finkle)
Accidentally added:
//selectTab(added.getId());

I could remove this (or the entire block, as the browser.js handles it anyways).
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.
Attachment #729620 - Flags: review?(mark.finkle) → review+
(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!
This doesn't search for the "Open in new tab" menu item.
Attachment #730970 - Flags: review?(bnicholson)
Comment on attachment 730970 [details] [diff] [review]
Patch: Remove tests in AwesomeBar

Looks good to me, but why abort the try build?
Attachment #730970 - Flags: review?(bnicholson) → review+
This removes everywhere.
Attachment #730970 - Attachment is obsolete: true
Attachment #731048 - Flags: review?(bnicholson)
Attachment #731048 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/c710854a413f
https://hg.mozilla.org/mozilla-central/rev/36eca1fb1c5c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Depends on: 869413
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.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.