88688 - (invalid) 'cursor: auto' shows text cursor for links

Status: VERIFIED INVALID

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Con Kolivas]

On the domain web site listed, the hyperlinks make the cursor change to the text
editor cursor instead of the hand hyperlink cursor. This happens on most links
on this web site. Browsing still works as normal, but it is not clear when you
are over a hyperlink.

Comment 1

[Greg Breland]

I see this on Win32 also, marking all.

The reason that mozilla is showing an I-bar cursor for all the links on this
page is because the stylesheet sets cursor:auto for all hyperlinks.  IE/NN4
display the hand icon.  I have no idea which is correct per spec, but the hand
would seems correct.

Attaching a reduced testcase.

Comment 2

[Greg Breland]

Attachment: Reduced Test Case

Comment 3

[Greg Breland]

See bug 30971

Looks like it caused this problem.  Marc Attinasi even deduced this bug when he
s=mattinasi, but I guess no one tested this case.

Comment 4

[Marc Attinasi]

This is definitely related to bug 30971, which was suppossed to make cursor:auto
work for everything but failed to make this case work. Could be dup'd to bug
30971 I think (I'm reopening bug 30971 now).

Comment 5

[joki (gone)]

I think its a dupe of 30971 but I'll let pierre make that call.

Comment 6

[Con Kolivas]

Found another site that exhibits this:
http://www.sea.vic.gov.au/renewable/SHW/rebate/index.html

Comment 7

[Con Kolivas]

Ok, now I'm not sure if this is the same bug but this time instead of a text
cursor, it changes to a bidirectional arrow over the menus on either side of the
main body of text and on the "go" buttons on this site:
http://www.itnews.com.au/
Linux build id 2001072621

Comment 8

[Pierre Saslawsky]

I'm going to attach a patch which is based on what we do in 
nsFrame::HandlePress().

Marc/DBaron: please review.  Would you prefer that code to be put into a new 
nsIFrame method that detects whether the current frame is inside an anchor?

Comment 9

[Pierre Saslawsky]

Attachment: patch 1.0

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How is this different from marking the "cursor: pointer" rule in html.css as
!important, and why do we want to fix this bug rather than WONTFIX it?

Comment 11

[Pierre Saslawsky]

If you set the cursor as !important in html.css, nobody will be able to change 
it, not even authors (UA-!important is as good as code and it always applies).  

The attached patch sets the cursor of a text frame only if the computed value of 
the cursor is still 'auto'.  If the frame is within an anchor with href, 'auto' 
is interpreted as 'pointer', otherwise it is interpreted as 'text'.

We want to fix it so that "cursor:auto" works correctly over both standard text 
and hyperlinks.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It still mostly defeats the point of having 'cursor: pointer' in html.css.  See
http://lists.w3.org/Archives/Member/w3c-css-wg/2001JulSep/0388.html

Comment 13

[Pierre Saslawsky]

|cursor:auto| doesn't mean "use the cursor that is defined for this element in 
html.css", it means "do the right thing if nothing else is specified".

- It doesn't defeat the point of having 'cursor: pointer' in html.css: it just 
means that the declarations in html.css are often redundant with what is already 
defined in C++.  The benefit for us of leaving the declarations in html.css is a 
greater flexibility if one day we decide to change the default cursors.  It also 
shows the user what to do if they want to override the declaration in a user 
stylesheet.

- |cursor:auto| also allows the cursor to switch automatically between values 
within the same element.  The typical case is 'text' and 'default' inside 
elements that contain some text, but it could also be the case inside complex 
form controls.  This behavior cannot usually be represented in a stylesheet 
(unless if the different zones of the element are selectable as pseudo-elements).

All in all, I see an interest in making |cursor:auto| work correctly.

Reviews please?

Comment 14

[Marc Attinasi]

Shouldn't this:

+      nsIContent* parent;
+      content->GetParent(parent);
+      content = dont_AddRef(parent);

be something like:

Pierre, this is strange:
+      content->GetParent(getter_AddRefs(content));

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, since the evaluation of the |this| for the GetParent call and the
constructor of the getter_AddRefs happen in an undefined order, and the latter
will happen first on Windows, causing a guaranteed null-dereference.

Comment 16

[Marc Attinasi]

OK, We do hat all the times with frames, but that is a different story.
r=attinasi on patch 1.0

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I still think this patch is wrong and I'd like to see this discussed in the
working group before we take this.  (The spec is *very* vague.)

Comment 18

[Pierre Saslawsky]

You already brought it up to the WG but nobody replied, maybe because they were 
like me a bit puzzled by the non-issues you described.  Could you rephrase why in 
your opinion the spec is vague and why this patch creates more problems than it 
fixes?  I don't get it. :-(

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Under the interpretation of 'cursor: auto' that you're supporting, this patch is
seriously incomplete -- it doesn't cover any of the form controls or other
things that have non-default cursors.  Following your approach correctly
requires a lot more code -- code that I'm sure will eventually be written and
bloat mozilla if we go down this path.  Under my interpretation, the 'cursor'
property is actually useful in UA stylesheets since we're not required to put
whatever is in the UA stylesheet into C++ code that interprets 'auto'.  I don't
see how an interpretation halfway between the two makes any sense, and I'd
prefer my interpretation to yours because it is vastly simpler.

Comment 20

[Pierre Saslawsky]

My interpretation is the following... The C++ code should set the cursor to 
'text' when the mouse is over some text, 'pointer' when the mouse is over a link, 
and 'default' everywhere else.  At any point, the cursor can be set through a 
style declaration, in ua.css or elsewhere.  That's what we currently have in 
Layout (look for the implementations of GetCursor()).  It doesn't seem very 
complex to me, nor likely to introduce some bloat.

What is your interpretation?

Comment 21

[Pierre Saslawsky]

DBaron: you proposed to mark this bug WontFix or to evangelize the offending site 
but...
1) We have more than one site showing the problem (the reporter listed three and 
I remember having seen this behavior on other sites).
2) Other browsers (Nav4 / IE) do the right thing and display the cursor as 
'pointer'.

Could someone please review?

Comment 22

[Marc Attinasi]

Comment on attachment 49870 [details] [diff] [review]
patch 1.0

reviewed on 2001-09-28 11:50

BTW: is there a perceptible performance hit with this? It looks like a bit of work to figure out the inAnchor value...

Comment 23

[Pierre Saslawsky]

No performance hit: it's only executed on mouse-move events over text frames.

waterson/jst: please review

Comment 24

[Johnny Stenback (:jst)]

This looks like a performance problem waiting to happen, the DOM tree you're
traversing could be *really* deep, how often is this code called? If this is
called every time the mouse cursor moves in a text frame, then we'll end up
calling this code a *lot*, and every call could potentially end up walking a
child-parent chain thousands of elements deep in edge cases (yes, there are such
edge cases out there).

The attached patch also only fixes this problem for HTML links, don't we have
the same problem with XLink links? I'm not all that excited about this patch, is
there a better alternative?

Comment 25

[Pierre Saslawsky]

jst: The patch merely does what we already do for nsFrame::HandlePress().  Do you 
think we should walk the frame tree instead of the content tree?  At least we 
would be limited by MAX_REFLOW_DEPTH.  I don't know about XLinks.  Can you write 
a testcase, or should I reassign to somebody else?

Comment 26

[Marc Attinasi]

I had to add support for SimpleXLinks in the Selector matching for :link way bak
when.  See
http://lxr.mozilla.org/seamonkey/source/content/shared/src/nsStyleUtil.cpp#679
for a clue on how to detect SimpleXLinks (note: it is expensive - we really need
a higher level link manager so we do not have to trundle through the content or
frame model to find out if something is a link - that was Hixie's idea.)

Comment 27

[Marc Attinasi]

Oh yea, please do not take my little minor, almost insignificant comment as an
indication that this bug should come to me :-D I have enough of my own - really.

Comment 28

[Pierre Saslawsky]

Here is an updated patch:
- XLinks are checked.
- It walks the frame tree.

attinasi/jst: please review.

Comment 29

[Pierre Saslawsky]

Attachment: patch 1.1

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I still don't think this should be checked in.  It will force all other browsers
to follow this silly behavior rather than putting it in the UA stylesheet (since
it would mean that both we and IE do it, and prevent any sensible solution from
happening in the future).

> 2) Other browsers (Nav4 / IE) do the right thing and display the cursor as 
> 'pointer'.

Do you seriously think Nav4 supported the 'cursor' property?

Comment 31

[Pierre Saslawsky]

|cursor:auto| is legit over links, there are pages out there doing it and you can 
even see it in some tutorials on the web [1] including from Microsoft.  The point 
is: Mozilla is the only browser that displays an i-beam cursor over these links.  
I don't think that we can close this bug as WONTFIX as you proposed and we 
certainly don't want to fix it with a UA-!important declaration.  It would be 
better if we had the higher level link manager mentioned above but it's not a 
reason to hold off fixing bugs.

[1] http://www.htmlgoodies.com/beyond/css_cursors.html
    http://www.see-search.com/webdesign/setcursor1.htm
    http://msdn.microsoft.com/library/en-us/dnwbld/html/mit9951.asp

Comment 32

[Marc Attinasi]

I have no opinion on whether this is good for the web or not - sorry, I'll leave
that to you guys. As for the code, it looks fine. If you decide to commit this
after considering David's arguments, sr=attinasi

Comment 33

[Pierre Saslawsky]

It's not the time to checkin trivial things like this.  We'll reconsider later.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 111870 has been marked as a duplicate of this bug. ***

Comment 35

[Kevin McCluskey (gone)]

Moving to mozilla1.1. Engineers are overloaded!

Comment 36

[Olav Vitters]

*** Bug 126590 has been marked as a duplicate of this bug. ***

Comment 37

[Kevin McCluskey (gone)]

Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling
work post Mozilla1.0.

Comment 38

[Boris Zbarsky [:bzbarsky]]

*** Bug 152534 has been marked as a duplicate of this bug. ***

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Assigning pierre's remaining Style System-related bugs to myself.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 160456 has been marked as a duplicate of this bug. ***

Comment 41

[Bill Mason]

*** Bug 168140 has been marked as a duplicate of this bug. ***

Comment 42

[Noel Clarkson]

Any updates on this? (it's been at least 1 year since anything was changed) 
This also causes a problem for afr.com (Australian Financial Review) which has
made the CFO of my company think that the site just doesn't work in Mozilla. 
I'm happy to argue that the problem occurs because the code is non standards
compliant - if that is the case,  but it just seems to be in limbo at the moment
(and has been for some time) as to whether it will be fixed or if it's a won't fix.

Is there a workaround for this?  I'm not too sure about the ua.css html.css
area, but if there is something I could add/change that would make things work
better, then that'd be good - but it seemed that there was some argument for and
some against this also (and I didn't quite follow the details of what it might
break by changing things, and I've never fiddled in this area, so I'm not really
sure what I'm doing in the first place).

Any sort of update would be good. 

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think our current behavior is correct.

Comment 44

[Elmar Ludwig]

*** Bug 226421 has been marked as a duplicate of this bug. ***

Comment 45

[Malcolm Rowe]

*** Bug 230369 has been marked as a duplicate of this bug. ***

Comment 46

[Hixie (not reading bugmail)]

Is still think this should be WONTFIX.

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

->invalid

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 253796 has been marked as a duplicate of this bug. ***

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 257495 has been marked as a duplicate of this bug. ***

Comment 50

[Prognathous]

Anyone care to explain what should tech evangelists suggest to websites that
have this problem?

Prog.

Comment 51

[Anne (:annevk)]

Those websites should add the following to their CSS file:

# a:link,a:visited{
#  cursor:pointer;
# }

Comment 52

[Prognathous]

Thanks Anne, do you have any suggestions for pages that script links using
onclick? This is another case where Mozilla displays an I-shaped cursor instead
of a hand-shaped one. See Bug 198349 for more details.

Prog.

Comment 53

[Anne (:annevk)]

They could have something like:

# *[onclick]{
#  cursor:pointer;
# }

The attribute selector won't work in Internet Explorer, but it will give the
intended effect in Mozilla.

Comment 54

[Mats Palmgren (inactive)]

*** Bug 271384 has been marked as a duplicate of this bug. ***

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 299301 has been marked as a duplicate of this bug. ***