Closed Bug 1055933 Opened 6 years ago Closed 5 years ago

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

Categories

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

31 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: silvo380, Assigned: ian.wills.9, Mentored)

Details

(Keywords: reproducible, testcase)

Attachments

(2 files, 6 obsolete files)

Attached file cursor_example.html (obsolete) —
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.
Confusingly, the testcase console.logs the correct value (pointer), but later inspection or manual evaluation logs the wrong value... (crosshair)
Status: UNCONFIRMED → NEW
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Attachment #8475695 - Attachment is obsolete: true
Attachment #8476696 - Attachment mime type: text/plain → text/html
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.
Mentor: bzbarsky
Component: CSS Parsing and Computation → DOM: CSS Object Model
Boris, are you saying that using the image frame for the style info is the incorrect thing to do in this case?
Could I get assigned to this bug? Been working with Boris and we are working on a solution.
Assignee: nobody → ian.wills.9
Attached patch rev1 (obsolete) — Splinter Review
Attachment #8560601 - Flags: review?(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?
Attachment #8560601 - Flags: review?(bzbarsky) → review+
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 on attachment 8560601 [details] [diff] [review]
rev1

># HG changeset patch
># Parent 58ce6051edf56ce70c1a62e88bd879a6e56d9239
># User Ian Wills <ian.wills.9@gmail.com>
>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
Attachment #8560601 - Flags: review+ → review?(bzbarsky)
Attached patch rev2 (obsolete) — Splinter Review
Attachment #8560704 - Flags: review?(bzbarsky)
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.
Attachment #8560601 - Flags: review?(bzbarsky)
Comment on attachment 8560704 [details] [diff] [review]
rev2

This looks great.  Please do add a test, though?
Attachment #8560704 - Flags: review?(bzbarsky) → review+
Attached patch bug1055933_fiximagemapstyle.diff (obsolete) — Splinter Review
Attachment #8560601 - Attachment is obsolete: true
Attachment #8560704 - Attachment is obsolete: true
Attachment #8562324 - Flags: review?(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.
Attachment #8562324 - Flags: review?(bzbarsky) → review+
Attached file test_bug1055933.html (obsolete) —
Good catch mPrimaryFrame wasn't getting set. This fixes it as we discussed.
Attachment #8562324 - Attachment is obsolete: true
Attachment #8563613 - Flags: review?(bzbarsky)
Attached patch bug1055933_fiximagemapstyle.diff (obsolete) — Splinter Review
Wrong file
Attachment #8563613 - Attachment is obsolete: true
Attachment #8563613 - Flags: review?(bzbarsky)
Attachment #8563617 - Flags: review?(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.
Attachment #8563617 - Flags: review?(bzbarsky) → review+
Stuff you mentioned above was extraneous so its been removed.
Attachment #8563617 - Attachment is obsolete: true
Attachment #8563811 - Flags: review?(bzbarsky)
Comment on attachment 8563811 [details] [diff] [review]
bug1055933_fiximagemapstyle.diff

r=me
Attachment #8563811 - Flags: review?(bzbarsky) → review+
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
https://hg.mozilla.org/mozilla-central/rev/143174851bb7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.