1015163 - Always show menu button in popup windows

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[nemo]

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

[:Gijs (he/him)]

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...

Comment 2

[nemo]

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

[:Gijs (he/him)]

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

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: 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).

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

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.

Comment 6

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/bc169301ff47

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Seems to me this was effectively dropped, let's only track it when you're actually spending time on it.

Comment 9

[Dão Gottwald [:dao]]

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

Comment 10

[:Gijs (he/him)]

(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. :-(

Comment 11

[nemo]

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?

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch

Picking this up again. The xul.css part isn't needed anymore given recent changes to the menu button.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c606f27cd0a
Always show the menu button in popup windows. r=gijs

Comment 14

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4a7f73f76c6a3ebd06d8eedb3795eee53d49944

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c606f27cd0a

Comment 16

[Mike Kaply [:mkaply]]

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

[:Gijs (he/him)]

(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.

Comment 18

[Mike Kaply [:mkaply]]

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.

Comment 19

[nemo]

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.

Comment 20

[Vlad Bacia-Mociran [:VladB]]

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.

Comment 21

[Brindusa Tot, Desktop QA]

Based on the previous comment, mark this bug Verified.