Zoom cursors don't appear with third party full theme or in framed images

RESOLVED FIXED in Firefox 23

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: Brandon Waterloo)

Tracking

({regression})

22 Branch
Firefox 23
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22- affected, firefox23-)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
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?
Flags: needinfo?(roc)
(Reporter)

Comment 2

5 years ago
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?
(Assignee)

Comment 3

5 years ago
Created attachment 738069 [details] [diff] [review]
Proposed patch v1.0

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.
Assignee: nobody → waterlo1
Status: NEW → ASSIGNED
Attachment #738069 - Flags: review?(jaws)
Flags: needinfo?(roc)

Updated

5 years ago
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
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).
Attachment #738069 - Flags: review?(jaws) → review+
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.
Attachment #738069 - Flags: review+ → review-
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.
Summary: Zoom cursors don't appear with third party full theme → Zoom cursors don't appear with third party full theme or in framed images
s/*could*/should/
(Reporter)

Comment 8

5 years ago
(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.
tracking-firefox22: ? → -
tracking-firefox23: ? → -
(Assignee)

Comment 11

5 years ago
Created attachment 738719 [details] [diff] [review]
Proposed patch v1.1

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.
Attachment #738069 - Attachment is obsolete: true
Attachment #738719 - Flags: feedback?(jaws)
(Reporter)

Comment 12

5 years ago
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?
Attachment #738719 - Flags: feedback+
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"))
>   }
> }
Attachment #738719 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 14

5 years ago
Created attachment 739149 [details] [diff] [review]
Proposed patch v1.2

This patch version includes feedback from jaws and Neil above.
Attachment #738719 - Attachment is obsolete: true
Attachment #739149 - Flags: review?(neil)
Attachment #739149 - Flags: review?(jaws)
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.
Attachment #739149 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 16

5 years ago
Created attachment 739192 [details] [diff] [review]
Proposed patch v1.3

This version incorporates feedback from jaws.
Attachment #739149 - Attachment is obsolete: true
Attachment #739149 - Flags: review?(neil)
Attachment #739192 - Flags: review?(roc)
(Reporter)

Comment 17

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

Comment 19

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

Comment 21

5 years ago
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.
Attachment #739192 - Flags: review?(roc) → review+
(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.
Attachment #739192 - Flags: review+
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.
Attachment #739192 - Flags: review+ → review-
(Assignee)

Comment 26

5 years ago
Created attachment 739749 [details] [diff] [review]
Proposed patch v1.4

This version incorporates jaws' feedback--moves .completeRotation to layout/style/TopLevelImageDocument.css.
Attachment #739192 - Attachment is obsolete: true
Attachment #739749 - Flags: review?(roc)
Attachment #739749 - Flags: review?(jaws)
Attachment #739749 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Attachment #739749 - Flags: review?(jaws)
Attachment #739749 - Flags: review+
(Assignee)

Comment 27

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

Comment 28

5 years ago
Created attachment 740127 [details] [diff] [review]
Proposed patch v1.5

This version fixes bitrot, otherwise identical to previous patch.
Attachment #739749 - Attachment is obsolete: true
Attachment #740127 - Flags: review?(roc)
Attachment #740127 - Flags: review?(jaws)
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.
Attachment #740127 - Flags: review?(jaws) → review+
Attachment #740127 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e822e8396c2
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e822e8396c2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

5 years ago
Depends on: 865767
You need to log in before you can comment on or make changes to this bug.