Always show menu button in popup windows

VERIFIED FIXED in Firefox 57

Status

()

defect
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: bugs, Assigned: dao)

Tracking

(Depends on 1 bug)

Trunk
Firefox 57
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox57 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
At present, unlike IE, there is no way to show a menu to offer print/save for popups without enabling toolbar=yes which is often unwanted.

Prior to Australis, under Windows, the firefox button was drawn in the window chrome, so was always available.

Prior to the firefox button, enabling menubar did the job.

Since the firefox button and ☰ button both offer menubar features, it seems it'd be nice if they respected that window.open hint to display themselves.

Maybe even hide if menubar=no? Dunno...

Also a problem in Chrome, FWIW, although Firefox and IE are the browsers over here.

https://code.google.com/p/chromium/issues/detail?id=82522

Comment 1

5 years ago
So I think this (ie showing the menu button if menubar=yes or toolbar=yes) makes sense to do because we used to show the fx button pre-Australis.

Dão and Madhava, do you agree?

The only reason I can think of not to do this is the fact that it's kind of strange if you customize the menu to remove things to the main toolbar, they will still be invisible, but they'll be visible in the menu panel, which is a little counterintuitive. I think that might be worth it considering the fact that right now, mouse users without the menubar can't easily print from these popups anymore, and we'd be fixing that in the default case...
Flags: needinfo?(madhava)
Flags: needinfo?(dao)
Reporter

Comment 2

5 years ago
Oh, this is interesting. IE9 (standard browser here) works fine, but IE11 has the exact same issue (no reasonable way to display print/save because menubar doesn't work)

So, same issue as the chrome bug and this bug.
Maybe a side effect of going w/ a chrome/ff layout.

So even once ff and chrome fix it, still going to have to do something about IE. print buttons are easy enough, save button doable, but kind of a PITA.  Here's hoping I can find a solution for IE first.

Comment 3

5 years ago
Redirecting Madhava's needinfo to Philipp, also because of the recently filed bug 1099590... Chrome has a context menu option for printing, which we've wontfixed in the past ( bug 204519), and it seems a number of users have issues with printing from popups. Yes, ideally websites shouldn't remove the toolbar/menubar in such cases... but ideally, websites would do a lot of things that they currently don't. :-(

Philipp, don't know how familiar you are with window.open - for documentation on the features string, see e.g. https://developer.mozilla.org/en-US/docs/Web/API/window.open#Toolbar_and_chrome_features
Flags: needinfo?(madhava) → needinfo?(philipp)
Posted image Mockup
Since it seems that we default to always show the location bar on popups, we might as well show the menu button next to it. Here's a mockup of what that could look like (showing the hover state of the menu button).
Flags: needinfo?(philipp)
Summary: consider showing the menu button for window.open'd windows without toolbar=yes but with menubar=yes → Always show menu button in popup windows
Assignee

Comment 5

5 years ago
Posted patch patch (obsolete) — Splinter Review
This should do it... The chained descendent selector in xul.css is pretty sad, but my only other idea would be to have browser.css explicitly set display:-moz-box for #PanelUI-popup .chromeclass-toolbar-additional, which doesn't seem great either.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Flags: needinfo?(dao)
Attachment #8534307 - Flags: review?(gijskruitbosch+bugs)
Attachment #8534307 - Flags: review?(enndeakin)
Assignee

Updated

5 years ago
Iteration: --- → 37.2
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+

Updated

5 years ago
Attachment #8534307 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

5 years ago
Attachment #8534307 - Flags: review?(enndeakin) → review+
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=2a11e49c5a83 since one of this 2 changes caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1424338&repo=fx-team
Flags: needinfo?(dao)
Iteration: 37.2 → 37.3
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Seems to me this was effectively dropped, let's only track it when you're actually spending time on it.
Iteration: 38.1 - 26 Jan → ---
Assignee

Comment 9

5 years ago
Yes, I'm not only effectively but actually dropping this. I hadn't had time to figure out what's needed to make /tests/dom/html/test/test_bug369370.html happy on Windows. Whoever feels like it should feel free to pick this up. For the record, the failures were:

TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_bug369370.html | Checking doc height - got 302, expected 300
TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_bug369370.html | Checking scrollTop - got 298, expected 300
Status: ASSIGNED → NEW
Flags: needinfo?(dao)
Assignee: dao → nobody

Comment 10

5 years ago
(In reply to Dão Gottwald [:dao] from comment #9)
> Yes, I'm not only effectively but actually dropping this. I hadn't had time
> to figure out what's needed to make /tests/dom/html/test/test_bug369370.html
> happy on Windows. Whoever feels like it should feel free to pick this up.
> For the record, the failures were:
> 
> TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_bug369370.html | Checking
> doc height - got 302, expected 300
> TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_bug369370.html | Checking
> scrollTop - got 298, expected 300

We've seen something similar in bug 872575, unfortunately it's not really clear what fixed it there. :-(
Reporter

Comment 11

4 years ago
Hrm. So this is dead due to some test fail, and a fail that somehow mysteriously fixed itself in another bug?  Could it be this one is ok now?
Assignee

Comment 12

2 years ago
Posted patch patchSplinter Review
Picking this up again. The xul.css part isn't needed anymore given recent changes to the menu button.
Assignee: nobody → dao+bmo
Attachment #8534307 - Attachment is obsolete: true
Status: NEW → ASSIGNED

Comment 13

2 years ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c606f27cd0a
Always show the menu button in popup windows. r=gijs

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c606f27cd0a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1390260, 1390273, 1390275
Was this decision reviewed by UX?

Now with the addition of the page actions on the popup window, we have even more actions that simply don't make sense in a popup window (and open new tabs which shoudn't be happening in a popup window).

Comment 17

2 years ago
(In reply to Mike Kaply [:mkaply] from comment #16)
> Was this decision reviewed by UX?

Yes, see comment #4.

> Now with the addition of the page actions on the popup window, we have even
> more actions that simply don't make sense in a popup window (and open new
> tabs which shoudn't be happening in a popup window).

Mike, you've been here a long time, you know this isn't a good way to report issues. Please file new bugs for page action items that cause issues. Note also that the page action menu appearing is separate from what we do for the hamburger button. Prior to bug 748894, the bookmarks star button also showed up in the location bar in popups (just like the popup reporter button and reader mode button are also supposed to "just work" in popup windows), so we're simply being consistent with how things used to work. In principle, I also see no reason why people shouldn't be able to bookmark, pocket, copy, email or share links/pages from popup windows, so showing the page action menu makes sense to me.
Flags: needinfo?(mozilla)
I'll open new bugs for the issues I found. I still fundamentally believe we've violated the toolbar=no/menubar=no contract for popup windows.
Flags: needinfo?(mozilla)
Reporter

Comment 19

2 years ago
A lot of those flags are kinda "legacy" names - but personally I'm just grateful you guys are finally offering something that honours the "yes" variant - up until now IE really was the only browser where a user could discover print who did not know the shortcut.  (nevermind print preview which often has no shortcut).

Also agree with him on most window options being totally legit in poups.

And, frankly, if you wanna be able to disable the master button, IMO it should be a brand new popup flag.
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170918100059

This issue has been verified on Mac OS 10.12, Windows 8.1 x64 and Ubuntu 14.04 with latest Nightly 57.0a1 and I didn't observed any issues. I have opened multiple popup windows from which I can print, copy-paste the content and also access the hamburger menu and the page action menu from URL bar without issues.
Based on the previous comment, mark this bug Verified.
Status: RESOLVED → VERIFIED
Assignee

Updated

2 years ago
Depends on: 1403853
You need to log in before you can comment on or make changes to this bug.