1055933 - Computed cursor style does not match actual cursor style for <area> elements

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[silvo380]

Attachment: cursor_example.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.68 Safari/537.36

Steps to reproduce:

Attached file has an example.

Relevant part of the code:

<div style="cursor: crosshair">
  <img usemap="#map" id="image" src="http://www.iconsdb.com/icons/preview/green/circle-xxl.png">
  <map id="map" name="map">
    <area id="area" shape="circle" coords="128,129,103" style="cursor: pointer">
  </map>
</div>

If you have an image-map, with a cursor style in the <area> element, and mouse over it, the cursor style is applied correctly. So in this example, mousing over the circle changes the cursor to the pointer style. 

However, this does not match the computed style for that element. In Javascript, check the computed style for that element (e.g. firebug console):

window.getComputedStyle(document.getElementById('area'));

Notice that the cursor style is "crosshair". Whereas the style should be pointer:

document.getElementById('area').style['cursor'];

And the pointer style is in fact the style of the cursor shown to the user.


Actual results:

The computed cursor style as returned by getComputedStyle does not return the correct value. Instead it returns the "crosshair" style inherited from the parent div.


Expected results:

The computed style as returned by getComputedStyle should match what the user actually sees when mousing over the image map (in this case, the "pointer" cursor style).

It looks like there is some inconsistent behaviour with getComputedStyle on <area> elements, where it is not checking the style of the element itself but is only looks at inherited values. Confusingly, in firebug if you inspect the <area> element, it shows the correct behaviour you would expect, where the pointer style overrides the crosshair style.

Comment 1

[:Gijs (he/him)]

Confusingly, the testcase console.logs the correct value (pointer), but later inspection or manual evaluation logs the wrong value... (crosshair)

Comment 2

[:Gijs (he/him)]

Attachment: Testcase showing changed style with console log

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is presumably due to us using the frame for style info.  For <area> elements the frame is the image frame, because <area> is a messed up puppy like that.

Comment 4

[Josh Matthews [:jdm]]

Boris, are you saying that using the image frame for the style info is the incorrect thing to do in this case?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Re comment 4, yes.

Comment 6

[ian.wills.9]

Could I get assigned to this bug? Been working with Boris and we are working on a solution.

Comment 7

[ian.wills.9]

Attachment: rev1

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8560601 [details] [diff] [review]
rev1

Ian, this looks pretty good but you probably want a space between the "//" and the "XXX" in the multiline comments and you definitely don't want the random whitespace changes to unrelated lines.  r=me with those issues fixed; please post an updated patch.

Also, are you willing to add a test for this?

Comment 9

[ian.wills.9]

Looking at the diff in Notepad++ it looks formatted correctly, so I'm not sure where the issue is. Should I use a different editor?
Sure I can add a test. An XPCShell test?

Comment 10

[ian.wills.9]

Comment on attachment 8560601 [details] [diff] [review]
rev1

># HG changeset patch
># Parent 58ce6051edf56ce70c1a62e88bd879a6e56d9239
># User Ian Wills <[e-mail]>
>Bug 1055933 - Fix Imagemaps Style in nsComputedDOMStyle.cpp
>
>diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
>--- a/layout/style/nsComputedDOMStyle.cpp
>+++ b/layout/style/nsComputedDOMStyle.cpp
>@@ -427,29 +427,32 @@ nsComputedDOMStyle::GetStyleContextForEl
>   // correct document.
>   nsIPresShell *presShell = GetPresShellForContent(aElement);
>   if (!presShell) {
>     presShell = aPresShell;
>     if (!presShell)
>       return nullptr;
>   }
> 
>-  if (!aPseudo && aStyleType == eAll) {
>+  // XXX the !aElement->IsHTML(nsGkAtoms::area)
>+  // check is needed due to bug 135040 (to avoid using 
>+  // mPrimaryFrame). Remove it once that's fixed.
>+  if (!aPseudo && aStyleType == eAll && !aElement->IsHTML(nsGkAtoms::area)) {
>     nsIFrame* frame = nsLayoutUtils::GetStyleFrame(aElement);
>     if (frame) {
>       nsStyleContext* result = frame->StyleContext();
>       // Don't use the style context if it was influenced by
>       // pseudo-elements, since then it's not the primary style
>       // for this element.
>       if (!result->HasPseudoElementData()) {
>         // this function returns an addrefed style context
>         nsRefPtr<nsStyleContext> ret = result;
>         return ret.forget();
>       }
>    }
>   }
> 
>   // No frame has been created, or we have a pseudo, or we're looking
>   // for the default style, so resolve the style ourselves.
>   nsRefPtr<nsStyleContext> parentContext;
>   nsIContent* parent = aPseudo ? aElement : aElement->GetParent();
>   // Don't resolve parent context for document fragments.
>   if (parent && parent->IsElement())
>@@ -589,33 +592,36 @@ nsComputedDOMStyle::UpdateCurrentStyleSo
>   mFlushedPendingReflows = aNeedsLayoutFlush;
> #endif
> 
>   mPresShell = document->GetShell();
>   if (!mPresShell || !mPresShell->GetPresContext()) {
>     return;
>   }
> 
>-  if (!mPseudo && mStyleType == eAll) {
>+  // XXX the !mContent->IsHTML(nsGkAtoms::area)
>+  // check is needed due to bug 135040 (to avoid using 
>+  // mPrimaryFrame). Remove it once that's fixed.
>+  if (!mPseudo && mStyleType == eAll && !mContent->IsHTML(nsGkAtoms::area)) {
>     mOuterFrame = mContent->GetPrimaryFrame();
>     mInnerFrame = mOuterFrame;
>     if (mOuterFrame) {
>       nsIAtom* type = mOuterFrame->GetType();
>       if (type == nsGkAtoms::tableOuterFrame) {
>         // If the frame is an outer table frame then we should get the style
>         // from the inner table frame.
>         mInnerFrame = mOuterFrame->GetFirstPrincipalChild();
>         NS_ASSERTION(mInnerFrame, "Outer table must have an inner");
>         NS_ASSERTION(!mInnerFrame->GetNextSibling(),
>                      "Outer table frames should have just one child, "
>                      "the inner table");
>       }
> 
>       mStyleContextHolder = mInnerFrame->StyleContext();
>       NS_ASSERTION(mStyleContextHolder, "Frame without style context?");
>     }
>   }
> 
>   if (!mStyleContextHolder || mStyleContextHolder->HasPseudoElementData()) {
> #ifdef DEBUG
>     if (mStyleContextHolder) {
>       // We want to check that going through this path because of
>       // HasPseudoElementData is rare, because it slows us down a good

Comment 11

[ian.wills.9]

Attachment: rev2

Comment 12

[Josh Matthews [:jdm]]

If you look at the previous diff in the Bugzilla view, you'll notice the odd formatting. That's caused by your editor using tab characters instead of spaces for indentation.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8560704 [details] [diff] [review]
rev2

This looks great.  Please do add a test, though?

Comment 14

[ian.wills.9]

Attachment: bug1055933_fiximagemapstyle.diff

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8562324 [details] [diff] [review]
bug1055933_fiximagemapstyle.diff

Shouldn't you check that the actual value returned both before and after the synthesizeMouse call is in fact "pointer"?

Also, I assume you've checked that the test fails if you don't make the C++ changes?

r=me if so and with explicit checks that the value is "pointer" added.

Comment 16

[ian.wills.9]

Attachment: test_bug1055933.html

Good catch mPrimaryFrame wasn't getting set. This fixes it as we discussed.

Comment 17

[ian.wills.9]

Attachment: bug1055933_fiximagemapstyle.diff

Wrong file

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8563617 [details] [diff] [review]
bug1055933_fiximagemapstyle.diff

r=me, but you don't need to synthesize a click on the <area>, and "first" is now unused and should go away.  Also:

>+ // Flush layout
>+document.body.offsetWidth;

This doesn't seem like it would do much, since it runs before the elements you're testing are present...  Doing it inside the requestAnimationFrame callback might make sense, though.

Comment 19

[ian.wills.9]

Attachment: bug1055933_fiximagemapstyle.diff

Stuff you mentioned above was extraneous so its been removed.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8563811 [details] [diff] [review]
bug1055933_fiximagemapstyle.diff

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d549ebdfa54

Comment 22

[Boris Zbarsky [:bzbarsky]]

The try failures in commet 21 seem to mostly be from bug 835800.  Pushed just this one to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1032fafdd9f4

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/143174851bb7

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/143174851bb7