Closed
Bug 1058243
Opened 10 years ago
Closed 10 years ago
Redundant, confusing link in Add-on Manager to AMO
Categories
(Firefox for Android Graveyard :: Add-on Manager, defect)
Tracking
(firefox35 verified, firefox36 verified)
VERIFIED
FIXED
Firefox 35
People
(Reporter: tecgirl, Assigned: tim_tim2000, Mentored)
References
Details
(Whiteboard: [lang=js][lang=html])
Attachments
(3 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
The Add-on manager has two navigation points to AMO. The first is an Extension icon with a caret at the top to the right of "Your Add-ons". Tapping this will take you to AMO. It's confusing since 1) It's next to the "Your Add-ons" title, 2) There's a cell with the same Extension icon with the title "Browse all Firefox Add-ons".
Proposal: Remove icon, caret and navigation point at the top of the Add-on manager.
Comment 1•10 years ago
|
||
Originally we didn't have that bottom row (added in bug 722902), but we probably should have gotten rid of that icon when we did that :)
To fix this bug, we'll need to edit these files:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.xhtml
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.js
Mentor: margaret.leibovic
Whiteboard: [lang=js][lang=html]
Assignee | ||
Comment 2•10 years ago
|
||
Assignee: nobody → tim_tim2000
Status: NEW → ASSIGNED
Attachment #8488995 -
Flags: review?(margaret.leibovic)
Comment 3•10 years ago
|
||
Comment on attachment 8488995 [details] [diff] [review]
Proposed patch for bug 1058243
Review of attachment 8488995 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the patch! r+ with one comment addressed.
Also, could you upload a screenshot for Robin to look at? If she thinks it looks okay, I can land an updated version of this patch for you.
::: mobile/android/chrome/content/aboutAddons.xhtml
@@ -31,5 @@
> </menu>
>
> <div id="addons-header" class="header">
> <div>&aboutAddons.header2;</div>
> - <div id="header-button" role="button" aria-label="&aboutAddons.browseAll;" pref="extensions.getAddons.browseAddons" />
You can also remove this string:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/aboutAddons.dtd#8
Attachment #8488995 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 4•10 years ago
|
||
I'm only wondering how do I compile Android version and install it on my phone in order to make a screenshot...
Attachment #8488995 -
Attachment is obsolete: true
Comment 5•10 years ago
|
||
(In reply to Timur Timirkhanov from comment #4)
> Created attachment 8492696 [details] [diff] [review]
> Patch with string deleted from dtd
>
> I'm only wondering how do I compile Android version and install it on my
> phone in order to make a screenshot...
I didn't realize you didn't make a build to test! Getting a build environment set up is the first thing you should do before jumping into writing patches.
You should follow the instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android
You should join #mobile on irc.mozilla.org to ask questions if you run into any problems getting a build set up. Good luck!
Comment 6•10 years ago
|
||
Comment on attachment 8492696 [details] [diff] [review]
Patch with string deleted from dtd
I tested this myself, and it looks good. I want to land this sooner rather than later, so I'll just land it. But still let me know if you want help getting a build set up for other bugs!
Attachment #8492696 -
Flags: review+
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
Had to back this out for test failures :(
https://hg.mozilla.org/integration/fx-team/rev/d67bd2471957
We need to update this test:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAddonManager.java#58
We can probably change it to just click on the "Browse all Firefox Add-ons" text instead of the icon in the top right corner.
Comment 9•10 years ago
|
||
I don't see much value to trying to make sure the "Browse all Firefox add-ons" link works, since we're not even checking to see if the URL we wanted loaded. It seems like the main value of this part of the test is to make sure we switch to an open add-on manager instance, rather than opening a new tab, so that's what I decided to do here.
Passed locally. Try push here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=66434116e444
Attachment #8499813 -
Flags: review?(michael.l.comella)
Comment 10•10 years ago
|
||
Comment on attachment 8499813 [details] [diff] [review]
(Part 2) Update testAddonManager to open a blank new tab instead of trying to click link in about:addons
Review of attachment 8499813 [details] [diff] [review]:
-----------------------------------------------------------------
Just one nit about updating the class comment.
::: mobile/android/base/tests/testAddonManager.java
@@ -42,5 @@
> // Load the about:addons page and verify it was loaded
> loadAndPaint(url);
> verifyPageTitle(StringHelper.ADDONS_LABEL);
>
> - // Change the AMO URL so we do not try to navigate to a live webpage
This was so fragile that I'm happy to see it removed. But you'll need to update the comment at the top of the test explaining what the test does; it no longer tries to tap the AMO link.
Attachment #8499813 -
Flags: review?(michael.l.comella) → review+
Comment 11•10 years ago
|
||
Thanks for the review, nalexander.
https://hg.mozilla.org/integration/fx-team/rev/c1cef1625a48
https://hg.mozilla.org/integration/fx-team/rev/92bcf77b60ed
https://hg.mozilla.org/mozilla-central/rev/c1cef1625a48
https://hg.mozilla.org/mozilla-central/rev/92bcf77b60ed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment 13•10 years ago
|
||
Verified as fixed in
Builds:
Firefox for Android 36.0a1 (2014-11-03)
Firefox for Android 35.0a2 (2014-11-03)
Device: Motorola Razr (Android 4.1.2)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•