1178047 - Cut and Copy menuitem are enabled when Nightly starts up

Status: REOPENED

(Core :: DOM: Core & HTML, defect)

Description

[u520171]

User Agent: Mozilla/5.0 (Windows NT 6.3; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150627030211

Steps to reproduce:

1) Confirm Cut and Copy menuitem are disabled or enabled after start up Firefox, Firefox Beta and Firefox Developer Edition. (Normally disabled)
2) Start up Nightly, then confirm Cut and Copy menuitem are disabled or enabled.

Environments:
Windows 8.1 (32 bit) / OS X 10.10 Yosemite
- Firefox Nightly 41.0a1 (2015-06-26)

The following build works well
- Firefox Developer Edition 40.0a2 (2015-06-26)
- Firefox 39.0 (2015-06-24)
- Firefox 38.0.5 (2015-05-25)


Actual results:

Cut and Copy menuitem are enabled on Nightly.


Expected results:

Cut and Copy menuitem are disabled.

Comment 1

[magicp]

This problem also has occurred on the following builds.

- Nightly 42.0a1 (20150629134017)
- Firefox Developer Edition 41.0b2 (20150629134017)

Comment 2

[magicp]

Attachment: menuitem-enabled-disabled.png

Please refer to the attached image.

Comment 3

[magicp]

Attachment: menuitem-enabled-disabled-noclip.png

Please refer to the attachment image
(No clipboard)

Comment 4

[Justin Dolske [:Dolske]]

Would be helpful to know exactly which Nightly build broke this.

One can do this by manually downloading and testing builds from https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/, or using mozregression -- a tool to help automate this: http://mozilla.github.io/mozregression/

Comment 5

[magicp]

(In reply to Justin Dolske [:Dolske] from comment #4)
> Would be helpful to know exactly which Nightly build broke this.
> 
> One can do this by manually downloading and testing builds from
> https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/, or using
> mozregression -- a tool to help automate this:
> http://mozilla.github.io/mozregression/

I confirmed that this problem occur on Nightly build 2015-05-27 and later.

app_name: firefox
build_date: 2015-05-27
build_path: k:\tmpc_9x4u\2015-05-27--mozilla-central--firefox-41.0a1.en-US.win32.zip
build_txt_url: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/05/2015-05-27-13-54-46-mozilla-central/firefox-41.0a1.en-US.win32.txt
build_type: nightly
build_url: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/05/2015-05-27-13-54-46-mozilla-central/firefox-41.0a1.en-US.win32.zip
changeset: ff2e07228041
pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c6bbf8f1b02b&tochange=ff2e07228041
repo: mozilla-central
repository: https://hg.mozilla.org/mozilla-central

Comment 6

[Alice0775 White]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dc683817edec&tochange=1921222e708e

Suspect:
 0431f570160d	Michael Layzell — Bug 1162952 - Return true from document.queryCommandEnabled('cut'/'copy') when in privileged or user-initiated code. r=ehsan

Comment 7

[Nika Layzell [:nika] (ni? for response)]

I believe that this is intended behavior. On firefox startup under the default configuration, your focus is in the about:home textbox. about:home is an HTML page, and in HTML pages we now need to always enable the cut and copy commands. Thus, they are enabled on startup.

The cut and copy commands should always be enabled in HTML pages, as it is now possible to intercept and handle the cut and copy events in web pages. As these events should be possible to fire whether or not there is a selection (and it is possible to copy values to the clipboard whether or not there is a selection), we must keep these commands enabled in case they are intercepted and handled by custom content javascript code.

I'm ni-ing ehsan to confirm that I'm correct on this.

Comment 8

[magicp]

(In reply to Michael Layzell [:mystor] from comment #7)
> I believe that this is intended behavior. On firefox startup under the
> default configuration, your focus is in the about:home textbox. about:home
> is an HTML page, and in HTML pages we now need to always enable the cut and
> copy commands. Thus, they are enabled on startup.
> 
> The cut and copy commands should always be enabled in HTML pages, as it is
> now possible to intercept and handle the cut and copy events in web pages.
> As these events should be possible to fire whether or not there is a
> selection (and it is possible to copy values to the clipboard whether or not
> there is a selection), we must keep these commands enabled in case they are
> intercepted and handled by custom content javascript code.
> 
> I'm ni-ing ehsan to confirm that I'm correct on this.

I have confirmed the cut and copy status (NOT all view) on the latest Nightly.
If you are correct, should fix [disabled] items?

[Enabled]
about:accounts
about:app-manager
about:buildconfig
about:cache
about:crashes
about:credits
about:healthreport
about:home
about:license
about:logo
about:memory
about:mozilla
about:networking
about:performance
about:plugins
about:privatebrowsing
about:rights
about:robots
about:serviceworkers
about:sessionrestore
about:support
about:sync-log
about:telemetry
about:webrtc
about:welcomeback
Developer Tools: searchbar in Rules, Computed
Developer Tools: code panes


[Disabled]
urlbar in nav-bar
searchbar in nav-bar
searchbar in Bookmarks sidebar
about:addons
about:config
about:newtab
about:preferences#general Home Page
about:preferences#search Keyword
about:preferences#content Exceptions -> Address of website
about:preferences#applications Searchbar
about:preferences#privacy Exceptions -> Address of website
about:preferences#privacy Show Cookies -> Searchbar
about:preferences#security Exceptions -> Address of website
about:preferences#advanced Network -> Connection Settings
about:permissions
about:sync-tabs
Developer Tools: searchbar in Inspector
Developer Tools: Web Console
Developer Tools: searchbar in Debugger
Developer Tools: searchbar in Network Monitor
Bookmark Library
History Library

Comment 9

[(no longer active)]

This is intended.  Neil Deakin asked us to disable this behavior for XUL documents which is why these entries are disabled in the pages you listed above.  There is no work remaining to be done here.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Based on comment 9, I do not feel the need to track this for FF41 and FF42.

Comment 11

[magicp]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #9)
> This is intended.  Neil Deakin asked us to disable this behavior for XUL
> documents which is why these entries are disabled in the pages you listed
> above.  There is no work remaining to be done here.

Why? Users don't know HTML or xul documents. Especially about:home and about:newtab. It's just strange behavior for users. That all.

Comment 12

[(no longer active)]

(In reply to magicp from comment #11)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #9)
> > This is intended.  Neil Deakin asked us to disable this behavior for XUL
> > documents which is why these entries are disabled in the pages you listed
> > above.  There is no work remaining to be done here.
> 
> Why? Users don't know HTML or xul documents. Especially about:home and
> about:newtab. It's just strange behavior for users. That all.

Asking Neil.  I have no strong opinions on this.

Comment 13

[Neil Deakin]

The issue is that HTML doesn't provide a means of indicating to the UI that the clipboard commands should be enabled. Rather than provide this, bug 1159490 just made them enabled all the time. Chrome does something similar. XUL does provide such a means so it should be used to ensure the UI is correct (as Chrome also does for the browser UI) We could do something similar though and use the right UI for about pages or chrome: files or something.

Note that in my opinion, the html behaviour is broken and bug 1159490 is invalid.

Comment 16

[Alice0775 White]

The problem appears only on Firefox.
Edge and Chrome is as expected, |copy||cut| contextmenu is disabled if nothing is selected in input field.

This bug means that Firefox is a liar. 

I think it need to re-consider this bug.

Comment 18

[Brendan Dahl [:bdahl]]

Nika,

As part of the load all XUL as XHTML work (bug 1550801), I've been running into issues with a number of copy/paste tests since we can no longer only filter XUL docs to allow if copy/paste in enabled.
I think we have two options going forward:

1. Change the checks that use to be IsHTMLOrXHTML to also check if the document is chrome privileged. If it is chrome privilege than we let it control copy/paste enable/disable.

or

2. We could get rid of the behavior that allows XUL to control copy/paste so it behaves like XHTML.

I lean towards #1, but wanted to get your feedback before I start updating tests.

Comment 19

[Nika Layzell [:nika] (ni? for response)]

(In reply to Brendan Dahl [:bdahl] from comment #18)

I lean towards #1, but wanted to get your feedback before I start updating tests.

I don't have any super strong feelings about this, so I would be OK with either approach. Given that our chrome HTML/XHTML is supposed to act like native UI, it probably makes sense to preserve the behaviour rather than returning to web behaviour.

I wonder if it would make sense to have this as a chrome-only attribute which can be set on the document or similar, rather than using the document's principal.

ni? :ehsan who helped me make the decision ~4 years ago when it first came up.

Comment 20

[(no longer active)]

See comment 13. The reason why this was previously tied to the document being XUL was that XUL provides the mechanism for the document to tell the UA whether copy/cut commands should be enabled or not through the XUL command controller infrastructure (hopefully I'm using the right terminology to refer to it... the code in dom/commandhandler is what I'm referring to). That infrastructure just doesn't exist in HTML at all.

Note that this has nothing to do with chrome privileges and is available to non-privileged XUL documents as well. So I don't think either of the ideas in (1) in comment 18 are exactly the equivalent of what we used to have, and (2) is non-desirable.

So Brendan, in response to your question, I'll ask, what is the plan for replacing the command controller infrastructure in HTML? Based on the answer to that question, this mechanism should be tied to where that replacement is available. I hope that makes sense. :-)

Comment 21

[Brendan Dahl [:bdahl]]

The command/broadcaster infrastructure has already been moved out of XUL and is supported in chrome privileged HTML too (bug 1486888). Whether we want to keep that around in the long term is a different question, but we do currently have the ability to control enable/disable in HTML.

Comment 22

[(no longer active)]

(In reply to Brendan Dahl [:bdahl] from comment #21)

The command/broadcaster infrastructure has already been moved out of XUL and is supported in chrome privileged HTML too (bug 1486888).

Hmm, based on https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/dom/xul/nsXULElement.cpp#691 this isn't entirely true... There is nothing in this code that looks at privileges that I can see. Where do we check to make sure this is restricted to chrome-privileged documents?

Whether we want to keep that around in the long term is a different question, but we do currently have the ability to control enable/disable in HTML.

Well, by HTML I meant HTML in the sense of the HTML spec. HTML + elements from the XUL namespace isn't really HTML. :-) (Not trying to nitpick over terminology of course, just clarifying what I meant... I specifically did not mean any document where IsHTMLOrXHTML() returns true for.)

Comment 23

[Brendan Dahl [:bdahl]]

(In reply to :Ehsan Akhgari from comment #22)

Hmm, based on https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/dom/xul/nsXULElement.cpp#691 this isn't entirely true... There is nothing in this code that looks at privileges that I can see. Where do we check to make sure this is restricted to chrome-privileged documents?

XUL elements can't be created (from content) in content privileged pages.

Well, by HTML I meant HTML in the sense of the HTML spec. HTML + elements from the XUL namespace isn't really HTML. :-) (Not trying to nitpick over terminology of course, just clarifying what I meant... I specifically did not mean any document where IsHTMLOrXHTML() returns true for.)

Yes, sorry to be clear I mean chrome privileged HTML.

Comment 24

[(no longer active)]

(In reply to Brendan Dahl [:bdahl] from comment #23)

(In reply to :Ehsan Akhgari from comment #22)

Hmm, based on https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/dom/xul/nsXULElement.cpp#691 this isn't entirely true... There is nothing in this code that looks at privileges that I can see. Where do we check to make sure this is restricted to chrome-privileged documents?

XUL elements can't be created (from content) in content privileged pages.

They can with the allowXULXBL permission, no? The actual condition isn't based on permissions at all: https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/dom/xul/nsXULElement.cpp#260.

Well, by HTML I meant HTML in the sense of the HTML spec. HTML + elements from the XUL namespace isn't really HTML. :-) (Not trying to nitpick over terminology of course, just clarifying what I meant... I specifically did not mean any document where IsHTMLOrXHTML() returns true for.)

Yes, sorry to be clear I mean chrome privileged HTML.

To the best of my knowledge, we currently have these two restrictions in Gecko:

• We create XUL elements where we grant the allowXULXBL permission, see the link above.
• We allow loading XBL bindings from system privileged or chrome:// URI stylesheets unless the document has allowXULXBL permission.

(Note that Document::AllowXULXBL() returns true for chrome-privileged documents.)

Since what bug 1486888 implemented needs to work where XUL elements can work, I think that feature needs to be gated on Document::AllowXULXBL() checks. I think that would make things work exactly the same way as they did in the browser.xul era. Can you please give that a shot and see if the tests you mentioned still fail with the fixes for bug 1550801?

Comment 25

[Brendan Dahl [:bdahl]]

For the time being, I tried gating on Document::AllowXULXBL instead, but then several other tests start failing (e.g. test_bug1067255.html) since they are plain html mochitests, but they use super powers (allowXULXBL is set to true). I still think it makes sense to gate this on system principal instead. We only really want this special behavior enabled for chrome documents and we don't need to support it in content documents. There is the case of content documents force enabling XUL/XBL, but that's only ever done in crashtests/reftests and some mochitests, so I don't see why we need to support that use case.

If you have another suggestion I'm open to trying other things.

Comment 26

[(no longer active)]

(In reply to Brendan Dahl [:bdahl] from comment #25)

For the time being, I tried gating on Document::AllowXULXBL instead, but then several other tests start failing (e.g. test_bug1067255.html) since they are plain html mochitests, but they use super powers (allowXULXBL is set to true). I still think it makes sense to gate this on system principal instead. We only really want this special behavior enabled for chrome documents and we don't need to support it in content documents. There is the case of content documents force enabling XUL/XBL, but that's only ever done in crashtests/reftests and some mochitests, so I don't see why we need to support that use case.

If you have another suggestion I'm open to trying other things.

Ugh, sorry, I really thought that would have worked, otherwise wouldn't have made you do the work. I suppose the good shouldn't be the enemy of the perfect here. You're right, since there is no need to support that use case, let's just gate this on the system principal-ness. Does that actually solve the problem at hand?