Closed Bug 597178 Opened 9 years ago Closed 9 years ago

about:addons has extra back-forward buttons

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: smaug, Assigned: mossop)

References

Details

Attachments

(1 file, 5 obsolete files)

The same buttons are also in the main UI and they do the same thing.
blocking2.0: --- → ?
...so please remove either one when showing about:addons.
Either bug 571970 or this one needs to be fixed before release I think.
Blocks: 571970
Intended, chrome will go away.
INVALID
(In reply to comment #3)
> Intended, chrome will go away.
> INVALID

Except that hasn't been done yet and is not a blocker so may not happen before Firefox 4.
In-Content UI will happen. Not in intended range, but it will. Confirmed be Stephen.
blocking2.0: ? → final+
What's the reason for blocking? Don't tell me even In-Conent UI won't make it at all...
(In reply to comment #6)
> What's the reason for blocking? Don't tell me even In-Conent UI won't make it
> at all...

Unless we fix bug 571970 and hide the main UI navigation buttons (and we haven't found a nice way to do that for all cases yet, and the bug isn't marked as blocking) then we have to do something here, so it should block the release.
Assignee: nobody → dtownsend
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Blocks: 591566
Attached patch patch rev 1 (obsolete) — Splinter Review
This removes the buttons and fixes the test, also re-enables it with a longer timeout which I think is all that is necessary. I'm unsure whether to remove the actual strings in case we decide to bring this back again.
Attachment #477710 - Flags: review?(bmcbride)
Status: NEW → ASSIGNED
There's likely some theme code that should be removed along with the buttons.
A bit OT, but since In-Content UI is scratched for 4.0, how do you intend to polish AOM? Because in current state it looks anyhow except good.
Attached patch patch rev 2 (obsolete) — Splinter Review
Indeed, this has the theme changes included.
Attachment #477710 - Attachment is obsolete: true
Attachment #477960 - Flags: review?(bmcbride)
Attachment #477710 - Flags: review?(bmcbride)
I've been thinking. Couldn't you make pseudo-In-Content UI by extending toolbar background into AOM?
(In reply to comment #11)
> Created attachment 477960 [details] [diff] [review]
> patch rev 2
> 
> Indeed, this has the theme changes included.

navigation.png should be removed too, from jar.mn at least.
Attached patch patch rev 3 (obsolete) — Splinter Review
Indeed, hopefully this one is good now
Attachment #477960 - Attachment is obsolete: true
Attachment #478285 - Flags: review?(bmcbride)
Attachment #478285 - Flags: feedback?
Attachment #477960 - Flags: review?(bmcbride)
Whiteboard: [has patch][needs review Unfocused]
Attachment #478285 - Flags: feedback? → feedback?(dao)
Attachment #478285 - Flags: feedback?(dao) → feedback+
Comment on attachment 478285 [details] [diff] [review]
patch rev 3

Given that the only time these would be added back is post-4.0 (and even then, only maybe), the strings should just be removed. Tracking down unused and forgotten strings is a real pain :(
Attachment #478285 - Flags: review?(bmcbride) → review-
Attached patch patch rev 4 (obsolete) — Splinter Review
Attachment #478285 - Attachment is obsolete: true
Attachment #479146 - Flags: review?(bmcbride)
Can I see screenshot with applied patch, please?
Attachment #479146 - Flags: review?(bmcbride) → review+
Whiteboard: [has patch][needs review Unfocused] → [has patch][needs-checkin-post-b7]
Fixed: http://hg.mozilla.org/mozilla-central/rev/4fa6d886a06e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch][needs-checkin-post-b7]
Target Milestone: --- → mozilla2.0b8
This checkin made the default branch and the b7 relbranch diverge in strings, which is something that can cause problems for locales - can we either have that landed for b7 or the string change reverted for the moment?
We've reverted the string change in http://hg.mozilla.org/mozilla-central/rev/41aded65fb6c. I'll talk with people tomorrow to see if it would be better to take it all on the relbranch too or to wait till after b7 has shipped to remove the strings from trunk.
Backed this out as apparently now bug 571970 is going to happen for just tabs-on-top so we need these here for that case. Need a new patch to hide buttons when the navigation bar is visible.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch new approach rev 1 (obsolete) — Splinter Review
This hides the navigation buttons based on context. If the manager is displayed in its own window or it is displayed in a frame where the top-level window doesn't have a visible element with ID back-btn then the buttons are visible, otherwise they are hidden. This also implements full history support for the windowed case.

I've also fixed bug 599891 here to have full testing of the windowed case. Any tests that use the UI are copied to a second testing directory where the head.js detects that window testing is wanted and enforces it. I haven't bothered with any magic to stop double-running all the tests in the case that the app they are running in doesn't support in-content. As far as I know only Seamonkey and Firefox run these tests and they both do.

I'll add further tests for ensuring the buttons appear/disappear when the toolbar/buttons appear/disappear in bug 571970, it is a waste of time to do that before then.
Attachment #479146 - Attachment is obsolete: true
Attachment #485079 - Flags: review?(bmcbride)
Whiteboard: [has patch][needs review Unfocused]
Blocks: 599891
Comment on attachment 485079 [details] [diff] [review]
new approach rev 1

>+  // Check whether to show the navigation buttons
>+  var docshellItem = window.QueryInterface(Ci.nsIInterfaceRequestor)
>+                           .getInterface(Ci.nsIWebNavigation)
>+                           .QueryInterface(Ci.nsIDocShellTreeItem);
>+
>+  // If there is no outer frame then leave the buttons visible
>+  if (docshellItem.rootTreeItem == docshellItem)
>+    return;
>+
>+  var outerWin = docshellItem.rootTreeItem.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                          .getInterface(Ci.nsIDOMWindow);
>+  var outerDoc = outerWin.document;
>+  var node = outerDoc.getElementById("back-button");
>+  // If the outer frame has no back-button then leave the buttons visible
>+  if (!node)
>+    return;
>+
>+  // If the back-button or any of its parents are hidden then leave the buttons
>+  // visible
>+  while (node != outerDoc) {
>+    var style = outerWin.getComputedStyle(node, "");
>+    if (style.display == "none")
>+      return;
>+    if (style.visibility != "visible")
>+      return;
>+    node = node.parentNode;
>+  }
>+
>+  // Otherwise there is an outer frame with a visible back-button so hide the
>+  // navigation buttons
>+  document.getElementById("back-btn").hidden = true;
>+  document.getElementById("forward-btn").hidden = true;
> }

Think this has the potential to flash the buttons - better to default to hidden, have have the code show them (I know this will probably make the code less nice).

If the browser back/forward buttons are shown/hidden while the addons manager is already loaded in a tab, its buttons won't update their visibility accordingly - but I don't think that's a big deal (and its quite an edge case). Also can't think of any way to reasonably detect that anyway.
Attachment #485079 - Flags: review?(bmcbride) → review-
Blocks: 606459
Split it out into a separate function to make that work ok, also made the buttons default to disabled to avoid that flickering
Attachment #485079 - Attachment is obsolete: true
Attachment #485331 - Flags: review?(bmcbride)
Comment on attachment 485331 [details] [diff] [review]
New approach rev 2

>+  if (shouldShowNavButtons()) {
>+    document.getElementById("back-btn").hidden = false;
>+    document.getElementById("forward-btn").hidden = false;
>+  }

Think it makes sense to move this into gHeader.initialize(), and have shouldShowNavButtons() be part of gHeader.

The rest looks good.
Attachment #485331 - Flags: review?(bmcbride) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/41e0b2a73fd8
Flags: in-testsuite- → in-testsuite+
Whiteboard: [has patch][needs review Unfocused]
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101029 Thunderbird/3.3a1pre and with latest Minefield builds on OS X and Windows like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101028 Firefox/4.0b8pre
Status: RESOLVED → VERIFIED
Depends on: 608586
Depends on: 607789
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.