Steps to reproduce problem: 1. Install a third party full theme (not background only) 2. View a large image 3. Zoom it in and out Expected result: Cursor changes as the image zooms Actual result: No zoom cursors at all

It appears that some themes (though not all of them--Noia Fox worked correctly for me) may not load the TopLevelImageDocument.css file, which contains the cursor styling for images, since it is implemented through a CSS class. I was able to reproduce the bug with Noia 4 (not to be confused with Noia Fox). Bug 846929 changed it from completely overwriting the CSS (which broke image rotation w/ gestures) to adding/removing a CSS class. There was an intermediate patch proposed that simply added/removed the appropriate CSS style, rather than overwriting the style attribute altogether; it looks like we'll need to go back to that method of adding/removing the particular style rule rather than adding/removing a class. Any thoughts, roc?

Originally I thought that the solution probably consists of moving the appropriate classes from toolkit/themes/*/global/media/TopLevelImageDocument.css to layout/style/TopLevelImageDocument.css but what about images in frames?

This patch moves the CSS classes relevant to layout/style/TopLevelImageDocument.css. This works on image documents using the default theme or when using a full third-party theme.

Comment on attachment 738069 [details] [diff] [review] Proposed patch v1.0 Review of attachment 738069 [details] [diff] [review]: ----------------------------------------------------------------- Note that this patch also moves the completeRotation class to the content styles since the browser front-end relies on this class being present (http://hg.mozilla.org/mozilla-central/diff/5e4ccad71f40/browser/base/content/browser-gestureSupport.js).

Comment on attachment 738069 [details] [diff] [review] Proposed patch v1.0 Review of attachment 738069 [details] [diff] [review]: ----------------------------------------------------------------- Sigh, I missed Neil's question about framed images. Yeah, this still needs to apply to framed images, so this patch won't do enough.

Regardless of a third-party theme installed, the cursor for > data:text/html,<iframe src="http://fc01.deviantart.net/fs70/f/2013/061/5/f/jawesome_by_mikecorriero-d5wqypd.jpg"> isn't shown because the associated stylesheet is only included if the image is not in a subframe. We *could* create a second stylesheet that is applied to all framed images. According to http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#read-media it appears acceptable per spec.

s/*could*/should/

(In reply to Jared Wein from comment #6) > We *could* create a second stylesheet that is applied to all framed images. We effectively already have one; it's created as an inline stylesheet in ImageDocument::CreateSyntheticDocument.

Oh, I thought that had been removed. I would prefer that we move that out to an external stylesheet (barring any noticeable perf hits).

No need to track, polish, nominate for uplift, if low risk, when ready.

Instead of moving CSS rules to layout/style/TopLevelImageDocument.css, a new file is created, layout/style/ImageDocument.css containing these rules. This CSS stylesheet is loaded unconditionally for ImageDocuments--so, even if they are in a frame, the stylesheet still gets loaded. The makefile is updated appropriately. Tested it with and without a third-party theme, and in frames.

Comment on attachment 738719 [details] [diff] [review] Proposed patch v1.1 I didn't see any behaviour issues with this patch applied. > if (!nsContentUtils::IsChildOfSameType(this) && > GetReadyStateEnum() != nsIDocument::READYSTATE_COMPLETE) { I don't know what the ready state check is for, so it may need to be tweaked. > LinkStylesheet(NS_LITERAL_STRING("resource://gre/res/TopLevelImageDocument.css")); > LinkStylesheet(NS_LITERAL_STRING("chrome://global/skin/media/TopLevelImageDocument.css")); > } >+ >+ LinkStylesheet(NS_LITERAL_STRING("resource://gre/res/ImageDocument.css")); >+ You may want to link the most general stylesheet first, in case we want to cascade styles at a later date. > _FILES = \ > contenteditable.css \ > designmode.css \ > TopLevelImageDocument.css \ > TopLevelVideoDocument.css \ >+ ImageDocument.css \ > $(NULL) Alphabetical order perhaps?

Comment on attachment 738719 [details] [diff] [review] Proposed patch v1.1 Review of attachment 738719 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to neil@parkwaycc.co.uk from comment #12) > Comment on attachment 738719 [details] [diff] [review] > Proposed patch v1.1 > > I didn't see any behaviour issues with this patch applied. > > > if (!nsContentUtils::IsChildOfSameType(this) && > > GetReadyStateEnum() != nsIDocument::READYSTATE_COMPLETE) { > I don't know what the ready state check is for, so it may need to be tweaked. See bug 775467 for an explanation. As I understand it, this patch needs to be refactored so that the new LinkStylesheet call isn't hit multiple times. It should be: > if (GetReadyStateEnum() != nsIDocument::READYSTATE_COMPLETE) { > LinkStylesheet(NS_LITERAL_STRING("resource://gre/res/ImageDocument.css")); > if (!nsContentUtils::IsChildOfSameType(this)) { > LinkStylesheet(NS_LITERAL_STRING("resource://gre/res/TopLevelImageDocument.css")); > LinkStylesheet(NS_LITERAL_STRING("chrome://global/skin/media/TopLevelImageDocument.css")) > } > }

This patch version includes feedback from jaws and Neil above.

Comment on attachment 739149 [details] [diff] [review] Proposed patch v1.2 Review of attachment 739149 [details] [diff] [review]: ----------------------------------------------------------------- You'll need review from :roc for the ImageDocument.cpp changes. ::: layout/style/ImageDocument.css @@ +23,5 @@ > +} > + > +@media print { > + /* We must declare the image as a block element. If we stay as > + an inline element, our parent LineBox will be inline too and This is now the same as http://mxr.mozilla.org/mozilla-central/source/layout/style/TopLevelImageDocument.css#27, so the same rule can be removed from the TopLevelImageDocument.css files.

This version incorporates feedback from jaws.

(In reply to Jared Wein from comment #15) > This is now the same as > http://mxr.mozilla.org/mozilla-central/source/layout/style/ > TopLevelImageDocument.css#27, so the same rule can be removed from the > TopLevelImageDocument.css files. That makes me wonder whether any of the other styles in TopLevelImageDocument.css actually belong in ImageDocument.css (I fear though that images in frames are badly broken at this point and a new bug is needed).

(In reply to neil@parkwaycc.co.uk from comment #17) > (In reply to Jared Wein from comment #15) > > This is now the same as > > http://mxr.mozilla.org/mozilla-central/source/layout/style/ > > TopLevelImageDocument.css#27, so the same rule can be removed from the > > TopLevelImageDocument.css files. > > That makes me wonder whether any of the other styles in > TopLevelImageDocument.css actually belong in ImageDocument.css (I fear > though that images in frames are badly broken at this point and a new bug is > needed). I double-checked and the other styles are in the correct place. We want to vertically and horizontally center the image for top-level image documents, but not for framed ones.

Indeed, but it was the margins that I was worried about.

Historically framed images have had the default page margins, which we don't want to break for sites that are depending on framed images to not change their appearance from version to version of Firefox.

Fair enough, but the zooming code doesn't seem to take that into account...

I think we shouldn't support zooming in framed images at all.

Comment on attachment 739192 [details] [diff] [review] Proposed patch v1.3 Review of attachment 739192 [details] [diff] [review]: ----------------------------------------------------------------- Having said that, this patch makes sense to me given we currently support zooming of these images.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > I think we shouldn't support zooming in framed images at all. Sounds fine to me, but we've supported it since at least 2006.

Comment on attachment 739192 [details] [diff] [review] Proposed patch v1.3 Review of attachment 739192 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/ImageDocument.css @@ +17,5 @@ > + cursor: -moz-zoom-in; > + } > + > + .completeRotation { > + transition: transform 0.3s ease 0s; Actually, this belongs in /layout/style/TopLevelImageDocument.css, not here since it doesn't get used for framed images.

This version incorporates jaws' feedback--moves .completeRotation to layout/style/TopLevelImageDocument.css.

Note: This patch is now suffering from some bit-rot. Will re-upload after I have a chance to test a new one.

This version fixes bitrot, otherwise identical to previous patch.

Comment on attachment 740127 [details] [diff] [review] Proposed patch v1.5 Review of attachment 740127 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. You should be able to carry-forward roc's r+, but I'll leave it for him since you already requested it.