Closed
Bug 852909
Opened 12 years ago
Closed 12 years ago
Character Encoding menu no longer disabled for images.
Categories
(Firefox :: Menus, defect)
Tracking
()
RESOLVED
FIXED
Firefox 28
Tracking | Status | |
---|---|---|
firefox20 | --- | unaffected |
firefox21 | - | --- |
firefox22 | - | --- |
People
(Reporter: beta, Assigned: hsivonen)
References
Details
(Keywords: regression)
Attachments
(5 files, 2 obsolete files)
671.18 KB,
image/png
|
Details | |
1.17 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.91 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
811 bytes,
patch
|
bdahl
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Comment 2•12 years ago
|
||
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
Updated•12 years ago
|
Severity: normal → minor
Keywords: regression
Updated•12 years ago
|
Component: Internationalization → Menus
Product: Core → Firefox
Comment 3•12 years ago
|
||
We'd consider a low risk uplift because this is a regression, but no need to track.
Comment 4•12 years ago
|
||
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•12 years ago
|
||
I'll take a look.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•12 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•12 years ago
|
||
Attachment #729718 -
Attachment is obsolete: true
Attachment #730670 -
Flags: review?(bugs)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #730671 -
Flags: review?(gavin.sharp)
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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 11•12 years ago
|
||
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•12 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.
Comment 13•12 years ago
|
||
(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•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #730671 -
Attachment is obsolete: true
Attachment #8336715 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 19•12 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•12 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•12 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)
Updated•12 years ago
|
Attachment #8336715 -
Flags: review?(gavin.sharp) → review+
Comment 22•12 years ago
|
||
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•12 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
Comment 24•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in
before you can comment on or make changes to this bug.
Description
•