Closed Bug 51113 Opened 24 years ago Closed 24 years ago

[FIX IN HAND,R=,A=] Wrong cursor on link mouseover because css cursor property is not inheriting

Categories

(Core :: DOM: HTML Parser, defect, P3)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: mlei2, Assigned: attinasi)

References

(
URL
)

Details

(Keywords: css2, Whiteboard: [nsbeta3-][fix in hand][PDTP3] [rtm++])

Attachments

(2 files)

test case
496 bytes, text/html
Details
Patch to fix cursor inheritance
941 bytes, patch
Details | Diff | Splinter Review 
Look at the code for this page: http://www.tjhsst.edu/~jdanaher

This is an excerpt from the code.

<td align=center width=24% valign=top>
<a href=blue/index.html><h2 align=center>Blue</h2></a>
My standard-issue personal homepage, with a random assortment of bits
and pieces. The original resident of this site. 
</td>

If you just look at this code, you'll see that the link is supposed to 
encompass the word "Blue."  But, if you go to this page, you will see that the 
link wrongly encompasses the words "Blue" thru "resident of this site."

I'm not sure what's wrong here. It seems to have something to do with the fact 
that the entire thing is inside a <td></td>. I'll attach a test case to 
demonstrate what I mean.

Running build 2000083111 Windows or 2000090108 Linux.
Attached file test case — 

    
    

    
  
You can't wrap a line-level element <a> around a block-level element <h2>. See 
http://www.w3.org/TR/html4/struct/global.html#h-7.5.3 for the rules. Since the 
specification is not absolutely clear on this, I will leave it for someone else 
to mark as invalid.

    
    

    
  
I don't think that the question is one of whether the syntax is exactly 
correct. Every browser must have some level of error tolerance-- in fact, 
having a high level of error tolerance is desirable because humans are not 
perfect and the browser should try to display "what I mean, not what I say
(write,code)."

The problem here is that in one case it renders as expected (when outside of a 
<td></td>) but in another case it does not (when inside a <td></td>). If one 
looked at the code, you would expect for the link to end at "Blue" even if the 
syntax was not exactly according to spec.

    
    

    
  
I can't make the decision, only the correct module owner can, but fault 
tolerance in code is not something I believe in. I believe people do make 
errors, but they can correct them. It is possible for the author to know what 
they meant, but in many cases it is not possible for the browser to know what 
they meant when they are doing illegal stuff.

    
    

    
  
mlei: No, error tolerance is a bad thing. It is what led us into the whole 
mess that the web is in now. Newer specifications such as CSS and XML follow
this view and actually say exactly how to deal with invalid things.

As you say -- every browser must have some level of error tolerance. Ours is
very low, as low as we can possibly make it without breaking the majority of
existing pages.

Harish, Rick: It seems that the content sink is using different rules for
when we are inside a TD and when we are not -- inside a TD, the </a> is being
ignored or something. Could you check it out quickly? If it is easy to fix then
we might as well fix it. If not, then let's mark it WONTFIX or INVALID.
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
requesting beta3 status because 1) it's dataloss; and 2) it's fixed in my tree.
Status: NEW → ASSIGNED
Keywords: dataloss, 
          
            nsbeta3
Whiteboard: [nsbeta3+]

    
    

    
  
Oops, wrong issue. I'd still like nsbeta3+ because we improperly form the 
document (and it's the most common case) and it's fixed in my tree. 
Keywords: dataloss

    
    

    
  
Fixed last week.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED

    
    

    
  
New bug:

Go to the test case (attachment 13902 [details]) or the URL above 
(http://www.tjhsst.edu/~jdanaher). If you move your mouse over the link, the 
cursor doesn't change to the hand but instead remains an I-text cursor. This is 
most likely related to the original bug that was fixed.

Build Windows 2000091008, Linux 2000090908.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: link not ending in the right place → Wrong cursor on link mouseover

    
    

    
  
Note: the original problem is fixed. The new problem concerns link handling 
ONLY. Here's a min test case:

<html><body>
<a href="foo"><h2>my heading</h2></a>
</body></html>

    
    

    
  
That is a violation of HTML4. Does Mozilla want to deal with invalid HTML? 
Inline elements must be contained within a block.

    
    

    
  
I'm really, really happy Joki is back to diagnose this. :)
Assignee: rickg → joki
Status: REOPENED → NEW

    
    

    
  
Okay, so I traced this through and we are walking through the correct frame 
which is associated with the anchor.  However, this frame, which is now a block 
frame as oppposed to an inline frame, isn't picking up the style rule for anchor 
as currently defined in html.css.  We probably need a new style rule to deal 
with block anchors.  Sending back to rickg.
Assignee: joki → rickg

    
    

    
  
This is a MAJOR issue. It happens *all* the time on pages, and is necessary for 
backward compatibility. The only remaining step is to improve our .css files.
Assignee: rickg → attinasi

    
    

    
  
Hmmm. The problem is that the cursor property is not inheriting, and according 
to the CSS2 spec it should. This can be fixed in code by altering the method 
StyleColorImpl::ResetFrom to inherit the cursor, or if we are too squeemish to 
change c++ code at this point some creative rules can do it too.

In html.css:

:link * { cursor: inherit; }

... Anything under a link will inherit the cursor. 

I think the best way is to fix the cursor inheritance code, that way other 
potential cursor bugs will be fixed as well, and we will be more adherant to the 
CSS2 spec. I'll attach a patch with the change to inherit the cursor, which 
fixes this bug (and potentially others).

Also, updating the summary to reflect the remaining problem.
Status: NEW → ASSIGNED
Summary: Wrong cursor on link mouseover → Wrong cursor on link mouseover because css cursor property is not inheriting
Target Milestone: --- → M18

    
    

    
  
Oops, I have other changes in that file so instead of a patch I'll just post the 
new version of the changed method:

void StyleColorImpl::ResetFrom(const nsStyleColor* aParent, nsIPresContext* 
aPresContext)
{
  if (nsnull != aParent) {
    mColor = aParent->mColor;
    mOpacity = aParent->mOpacity;
    mCursor = aParent->mCursor; /* inherit the cursor from parent */
  }
  else {
    if (nsnull != aPresContext) {
      aPresContext->GetDefaultColor(&mColor);
    }
    else {
      mColor = NS_RGB(0x00, 0x00, 0x00);
    }
    mOpacity = 1.0f;
    mCursor = NS_STYLE_CURSOR_AUTO;  
  }

  mBackgroundFlags = NS_STYLE_BG_COLOR_TRANSPARENT | NS_STYLE_BG_IMAGE_NONE;
  if (nsnull != aPresContext) {
    aPresContext->GetDefaultBackgroundColor(&mBackgroundColor);
    aPresContext->GetDefaultBackgroundImageAttachment(&mBackgroundAttachment);
    aPresContext->GetDefaultBackgroundImageRepeat(&mBackgroundRepeat);
    aPresContext->GetDefaultBackgroundImageOffset(&mBackgroundXPosition, 
&mBackgroundYPosition);
    aPresContext->GetDefaultBackgroundImage(mBackgroundImage);
  }
  else {
    mBackgroundColor = NS_RGB(192,192,192);
    mBackgroundAttachment = NS_STYLE_BG_ATTACHMENT_SCROLL;
    mBackgroundRepeat = NS_STYLE_BG_REPEAT_XY;
    mBackgroundXPosition = 0;
    mBackgroundYPosition = 0;
  }
}


    
    

    
  


  

    
    

    
  
I attached the patch after all to make testing and reviewing easier, and because 
I had to get the other changes out anyway if I plan to commit this. 

General Idea: get the cursor from the parent if there is a parent, otherwise set 
it to the defaul value of auto.

    
    

    
  
Bumping up to P2 based upon Rick G.'s comments.
Keywords: css2
Priority: P3 → P2
Whiteboard: [nsbeta3+] → [nsbeta3+][fix in hand]

    
    

    
  
The patch is fine but I believe that it only fixes part of the problem. It seems 
to me that nsTextFrame::GetCursor() should iterate through all the parents (not 
just the first one) to find the first one which doesn't have NS_STYLE_CURSOR_AUTO 
(and if we don't find any, set NS_STYLE_CURSOR_TEXT), a mechanism a little bit 
similar to what we have for 'cursor-select' in nsFrame::IsSelectable().

When it's done, could you make sure that bug 30971 (cursor: auto does not work) 
and bug 20080 (cursor: inheritance bug: not inherited inside floated span) are 
still valid? Thanks.


    
    

    
  
PDT thinks this is an edge case, ->P3.
Priority: P2 → P3
Whiteboard: [nsbeta3+][fix in hand] → [nsbeta3+][fix in hand][PDTP3]

    
    

    
  
per PDT: P3-P5 priority bugs changed from nsbeta3+ to nsbeta3- since we have
more important work to do for Seamonkey. If you disagree, please state your case
in the bug report and nominate for rtm. Thanks.

Whiteboard: [nsbeta3+][fix in hand][PDTP3] → [nsbeta3-][fix in hand][PDTP3]

    
    

    
  
Agreed that beta3 can live without this. However, inheriting the cursor property
should get in for RTM: it is very low risk, and it is correct behavior.

The additional problem that Pierre brought up (walking ancestors to find
non-auto cursor for text frames) can be treated as a separate bug if needed.

Keywords: rtm

    
    

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

    
    

    
  
Adding rtm+ due to low risk.
Whiteboard: [nsbeta3-][fix in hand][PDTP3] → [nsbeta3-][fix in hand][PDTP3] [rtm+]

    
  
Keywords: patch
Summary: Wrong cursor on link mouseover because css cursor property is not inheriting → [FIX IN HAND] Wrong cursor on link mouseover because css cursor property is not inheriting

    
    

    
  
PDT marking [rtm need info] since the patch doesn't have code review yet. 
Whiteboard: [nsbeta3-][fix in hand][PDTP3] [rtm+] → [nsbeta3-][fix in hand][PDTP3] [rtm need info]

    
    

    
  
Pierre said the patch was fine, though he did bring up a related problem. See
his comment from 2000-09-15 13:52 and my response vis a vis opening another bug
for the related issue from 2000-09-26 14:07.

I'm requesting super-review for this now.


    
    

    
  
I believe that the auto-cursor handling that Pierre mentioned in his comment
from 2000-09-15 13:52 is covered by bug 30971. I'm copying his comment to that
bug so we don't lose the information.


    
    

    
  
r=pierre. This fix is sound and safe. The rest will be fixed later. Marc moved my 
comments to bug 30971.


    
    

    
  
add comments to the fix citing the bug number, and you get a=buster


    
    

    
  
Have r=a and a=, remarking rtm+ and waiting for PDT approval

Summary: [FIX IN HAND] Wrong cursor on link mouseover because css cursor property is not inheriting → [FIX IN HAND,R=,A=] Wrong cursor on link mouseover because css cursor property is not inheriting
Whiteboard: [nsbeta3-][fix in hand][PDTP3] [rtm need info] → [nsbeta3-][fix in hand][PDTP3] [rtm+]

    
    

    
  
PDT marking [rtm++]

Whiteboard: [nsbeta3-][fix in hand][PDTP3] [rtm+] → [nsbeta3-][fix in hand][PDTP3] [rtm++]

    
    

    
  
Fix checked into branch and trunk (nsStyleContext.cpp)

Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED

    
    

    
  
verified: 

2000-10-19-08-Mtrunk : win32 
2000-10-19-09-MN6 : win32 

2000-10-19-09-MN6 : linux
 
2000-10-19-08-MN6 : mac 
 
Status: RESOLVED → VERIFIED

    
    

    
  
Mass removing self from CC list.

    
    

    
  
Now I feel sumb because I have to add back. Sorry for the spam.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: