Closed Bug 1015163 Opened 6 years ago Closed 3 years ago

Always show menu button in popup windows

Categories

(Firefox :: Menus, defect)

defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: bugs, Assigned: dao)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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
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)
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.
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)
Attached 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
Attached 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)
Iteration: --- → 37.2
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Attachment #8534307 - Flags: review?(gijskruitbosch+bugs) → review+
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 → ---
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
(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. :-(
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?
Attached 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
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c606f27cd0a
Always show the menu button in popup windows. r=gijs
https://hg.mozilla.org/mozilla-central/rev/6c606f27cd0a
Status: ASSIGNED → RESOLVED
Closed: 3 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).
(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)
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
Depends on: 1403853
You need to log in before you can comment on or make changes to this bug.