Character Encoding menu no longer disabled for images.

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Menus
--
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: beta, Assigned: hsivonen)

Tracking

({regression})

21 Branch
Firefox 28
x86_64
All
regression
Points:
---

Firefox Tracking Flags

(firefox20 unaffected, firefox21-, firefox22-)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 727130 [details]
Firefox showing the menu.
Last good nightly: 2013-02-01
First bad nightly: 2013-02-02

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=50cf5bbcb180&tochange=4e7c92906a79
Blocks: 234628
Status: UNCONFIRMED → NEW
Component: Untriaged → Internationalization
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
Severity: normal → minor
Keywords: regression

Updated

5 years ago
status-firefox20: --- → unaffected
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
Component: Internationalization → Menus
Product: Core → Firefox

Comment 3

5 years ago
We'd consider a low risk uplift because this is a regression, but no need to track.
tracking-firefox21: ? → -
tracking-firefox22: ? → -
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?
(Assignee)

Comment 5

5 years ago
I'll take a look.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 6

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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?
(Assignee)

Comment 7

5 years ago
Created attachment 730670 [details] [diff] [review]
Make media docs report they ignore overrides
Attachment #729718 - Attachment is obsolete: true
Attachment #730670 - Flags: review?(bugs)
(Assignee)

Comment 8

5 years ago
Created attachment 730671 [details] [diff] [review]
Stop observing uselessly
Attachment #730671 - Flags: review?(gavin.sharp)
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 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.
Attachment #730670 - Flags: review?(bugs) → review+
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).
Attachment #730671 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 12

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(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.
(Assignee)

Comment 14

4 years ago
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 16

4 years ago
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).
Attachment #8336711 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 17

4 years ago
Created attachment 8336714 [details] [diff] [review]
Disable the menu for PDF.js
(Assignee)

Comment 18

4 years ago
Created attachment 8336715 [details] [diff] [review]
Stop observing uselessly, unrotted
Attachment #730671 - Attachment is obsolete: true
Attachment #8336715 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 19

4 years ago
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.
Attachment #8336714 - Flags: review?(bdahl)

Comment 20

4 years ago
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
Attachment #8336711 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 21

4 years ago
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.
Attachment #8336715 - Flags: review?(gijskruitbosch+bugs) → review?(gavin.sharp)
Attachment #8336715 - Flags: review?(gavin.sharp) → review+
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
Attachment #8336714 - Flags: review?(bdahl) → review+
(Assignee)

Comment 23

4 years ago
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.