852909 - Character Encoding menu no longer disabled for images.

Status: RESOLVED FIXED

Description

[John Drinkwater (:beta)]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:21.0) Gecko/20130319 Firefox/21.0
Build ID: 20130319042012

Steps to reproduce:

Visited a PNG image, noticed the character encoding menu is enabled.
Visited a SVG image (with charset), menu is correctly disabled.

Looks like it may be from the change in Bug 234628 as this does behave correctly in Firefox 19.



Expected results:

It should have disabled the menu item for images.

Comment 1

[John Drinkwater (:beta)]

Created attachment 727130 [details]
Firefox showing the menu.

Comment 2

[Matthias Versen [:Matti]]

Last good nightly: 2013-02-01
First bad nightly: 2013-02-02

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=50cf5bbcb180&tochange=4e7c92906a79

Comment 3

[Alex Keybl [:akeybl]]

We'd consider a low risk uplift because this is a regression, but no need to track.

Comment 4

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

Created attachment 729718 [details] [diff] [review]
patch

I think the core problem is that the code in updateCharacterEncodingMenuState conflicts with the attribute broadcasting done by the isImage observer. So let's just have this particular menu item stop observing isImage, and instead use custom code in updateCharacterEncodingMenuState to disable itself appropriately.

Could use a test (and I also haven't tested this).

hsivonen, could you take this?

Comment 5

[Henri Sivonen (:hsivonen)]

I'll take a look.

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> Created attachment 729718 [details] [diff] [review]
> patch
> 
> I think the core problem is that the code in
> updateCharacterEncodingMenuState conflicts with the attribute broadcasting
> done by the isImage observer. So let's just have this particular menu item
> stop observing isImage, and instead use custom code in
> updateCharacterEncodingMenuState to disable itself appropriately.

Actually, the problem is that I thought media documents didn't inherit from HTML docs and forgot to re-override WillIgnoreCharsetOverride in media docs.

> Could use a test (and I also haven't tested this).

I'm not properly familiar with front end testing. How should I write a test for this?

Comment 7

[Henri Sivonen (:hsivonen)]

Created attachment 730670 [details] [diff] [review]
Make media docs report they ignore overrides

Comment 8

[Henri Sivonen (:hsivonen)]

Created attachment 730671 [details] [diff] [review]
Stop observing uselessly

Comment 9

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

I thought about that solution too (changing what mayEnableCharacterEncodingMenu returns for media documents), but that results in a bit of a behavior change compared to the previous code (which checked for a text-based MIME type, which also captures things like full-page plugins and other non-media, non-text stuff). I guess maybe that's not a big deal, but being inconsistent about when we disabled the Character Set menu and the View Source menu seemed sub-optimal.

Comment 10

[Olli Pettay [:smaug] (high review load)]

Comment on attachment 730670 [details] [diff] [review]
Make media docs report they ignore overrides

Whether or not this helps with the UI, this seems like the right thing to do.

{ should go to the next line.

Comment 11

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

Comment on attachment 730671 [details] [diff] [review]
Stop observing uselessly

r- given comment 9, I think we should avoid changing behavior here (though as smaug says the back-end patch is desirable anyways).

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> r- given comment 9, I think we should avoid changing behavior here

What behavior do you not want changed? What behavior would stopping observing change? AFAICT, the observation removal is just clean-up after the Gecko-side change changes behavior in the desired way (causes the menu to be disabled for image, plug-in, video and audio docs).

Note that Character Encoding and View Source don't enable/disable in unison anyway, because View Source makes sense for XML but Character Encoding has no effect for XML.

I still don't know how I should write a test for this.

Comment 13

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

(In reply to Henri Sivonen (:hsivonen) from comment #12)
> What behavior do you not want changed?

Changing behavior" relative to prior to your patch for bug 234628, to be clear. "document is a MediaDocument" and "mimeTypeIsTextBased(document.contentType) returns true" aren't quite the same thing. Maybe they are closer than I thought, though, in which case maybe your patch is fine.

With your patches, things like PDF documents (and, I thought, full-page plugins) will have enabled Character Encoding menus, whereas prior to bug 234628 they would not have.

> AFAICT, the observation removal is just clean-up after the
> Gecko-side change changes behavior in the desired way (causes the menu to be
> disabled for image, plug-in, video and audio docs).

(I didn't think plug-ins were MediaDocuments.)

> I still don't know how I should write a test for this.

Write a browser-chrome test that loads an image document, and check that the character encoding menu is disabled. browser_zbug569342.js does something similar (for the find bar), but could be simplified a lot for this simpler case.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #13)
> With your patches, things like PDF documents (and, I thought, full-page
> plugins) will have enabled Character Encoding menus, whereas prior to bug
> 234628 they would not have.

With this patch, the Character Encoding menu becomes disabled when the top-level document is a plug-in. However, the patch enabled the menu for PDF.js. :-(

> (I didn't think plug-ins were MediaDocuments.)

PluginDocument inherits from MediaDocument.

Comment 15

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #14)
> However, the patch enabled the menu for PDF.js. :-(

By far the easiest fix for this is to start the PDF.js viewer.html with a UTF-8 BOM.

Comment 16

[Henri Sivonen (:hsivonen)]

Created attachment 8336711 [details] [diff] [review]
Test case

The binary patches are a 16x16-pixel green square as a PNG and as a PDF (converted from the PNG using imagemagick).

Comment 17

[Henri Sivonen (:hsivonen)]

Created attachment 8336714 [details] [diff] [review]
Disable the menu for PDF.js

Comment 18

[Henri Sivonen (:hsivonen)]

Created attachment 8336715 [details] [diff] [review]
Stop observing uselessly, unrotted

Comment 19

[Henri Sivonen (:hsivonen)]

Comment on attachment 8336714 [details] [diff] [review]
Disable the menu for PDF.js

This patch adds a BOM. Hence, the change looks invisible. At least another patch has a test case in case this regresses, since this is easy to regress. However, this is also by far the easiest way to disable the Character Encoding menu for PDF.js.

Comment 20

[:Gijs (he/him)]

Comment on attachment 8336711 [details] [diff] [review]
Test case

Review of attachment 8336711 [details] [diff] [review]:
-----------------------------------------------------------------

This looks alright. Please push the whole thing (with fix) to try when it's all reviewed, as the paths to external files thing makes me somewhat uncomfortable, but I'm not aware of a better way of doing this.

::: docshell/test/browser/browser_bug852909.js
@@ +18,5 @@
> +  gBrowser.removeCurrentTab();
> +  gBrowser.selectedTab = gBrowser.addTab(rootDir + "file_bug852909.pdf");
> +  gBrowser.selectedBrowser.addEventListener("load", pdf, true);
> +}
> +  

Nit: whitespace

Comment 21

[:Gijs (he/him)]

Comment on attachment 8336715 [details] [diff] [review]
Stop observing uselessly, unrotted

Given that gavin r-'d last time around, I think it should be him reviewing this as well.

Comment 22

[Brendan Dahl [:bdahl]]

Comment on attachment 8336714 [details] [diff] [review]
Disable the menu for PDF.js

Review of attachment 8336714 [details] [diff] [review]:
-----------------------------------------------------------------

Upstreamed the change as well https://github.com/mozilla/pdf.js/pull/3957

Comment 23

[Henri Sivonen (:hsivonen)]

Thanks everyone!

(In reply to :Gijs Kruitbosch from comment #20)
> Please push the whole thing (with fix) to try when it's all reviewed

https://tbpl.mozilla.org/?tree=Try&rev=4024f26b425b

> Nit: whitespace

Fixed.

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40034bbdace1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3538da572ad3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85ba1d0caee
https://hg.mozilla.org/integration/mozilla-inbound/rev/99479edbee2a

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40034bbdace1
https://hg.mozilla.org/mozilla-central/rev/3538da572ad3
https://hg.mozilla.org/mozilla-central/rev/a85ba1d0caee
https://hg.mozilla.org/mozilla-central/rev/99479edbee2a