Bug 39315 - selections in margins wrong with div followed by a span
Summary: selections in margins wrong with div followed by a span
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-18 13:50 PDT by Ojan Vafai
Modified: 2010-06-09 16:31 PDT (History)
6 users (show)

See Also:


Attachments
test case (177 bytes, text/html)
2010-05-18 13:50 PDT, Ojan Vafai
no flags Details
I have to push this bug by committing a patch. :) (16.85 KB, patch)
2010-06-02 21:06 PDT, Robin Qiu
ojan: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-05-18 13:50:25 PDT
Created attachment 56409 [details]
test case

See test case. Originally seen on http://www.lewiz.org/2010/05/chrome-incognito-tracks-visited-sites.html.
Comment 1 Robin Qiu 2010-05-28 01:28:42 PDT
hi Ojan, I have investigated this bug. It's cause by this line:
http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderBlock.cpp#L3783
here, childBox->frameRect().bottom() is used, but, I think, childBox->maxBottomMargin(true) is more suitable. In that case, webkit acts similarly with FF.

I would like to hear other people before I prepare a patch for this bug.
Comment 2 Robin Qiu 2010-05-31 03:47:09 PDT
It seems that FF use half of the margin as the boundary of this two elements. 
Should I change this code 
if (isChildHitTestCandidate(childBox) && contentsY < childBox->frameRect().bottom())
to 
if (isChildHitTestCandidate(childBox) && contentsY < childBox->frameRect().bottom() +  childBox->maxBottomMargin(true) / 2)
?

Is there any specification defines this?
Comment 3 Robin Qiu 2010-06-02 21:06:02 PDT
Created attachment 57730 [details]
I have to push this bug by committing a patch. :)
Comment 4 Ojan Vafai 2010-06-08 17:32:10 PDT
Comment on attachment 57730 [details]
I have to push this bug by committing a patch. :)

(In reply to comment #2)
> It seems that FF use half of the margin as the boundary of this two elements. 
> Should I change this code 
> if (isChildHitTestCandidate(childBox) && contentsY < childBox->frameRect().bottom())
> to 
> if (isChildHitTestCandidate(childBox) && contentsY < childBox->frameRect().bottom() +  childBox->maxBottomMargin(true) / 2)
> ?

How does this fix the bug? Doesn't a similar bug still happen? Instead of happening immediately after the div in the test case, it happens halfway through the margin of the div in the test case. In other words, when you select past the half-way point, but still above the last line, the last line gets selected and it shouldn't. Am I wrong? Does this patch fix that case?

> Is there any specification defines this?

There is not a specification for this. It is up to the user-agent. We've tried to match IE/FF where it makes sense to avoid making an arbitrarily different decision for no reason and to match general user-expectations (that assumes users are used to the IE/FF behavior). It's not clear to me whether we should match Firefox here or not, but in either case, it's a separate bug.
Comment 5 Robin Qiu 2010-06-08 21:22:30 PDT
(In reply to comment #4)
I don't think this bug is a 'real' bug, becuase no spec indicates how to deal with this case.  I committed this patch just becuase I thought FF's behavior are more reasonable than WebKit. After patching this patch, WebKit will act similarly with FF.

All in all, this is not a technical bug, I think it can be closed without any modification.
Comment 6 Ojan Vafai 2010-06-09 16:31:13 PDT
(In reply to comment #5)
> (In reply to comment #4)
> I don't think this bug is a 'real' bug, becuase no spec indicates how to deal with this case.  I committed this patch just becuase I thought FF's behavior are more reasonable than WebKit. After patching this patch, WebKit will act similarly with FF.
> 
> All in all, this is not a technical bug, I think it can be closed without any modification.

Actually, it looks like the original bug is fixed. Not sure which revision it was. Try the test case in a WebKit nightly versus a Safari 4 or Chrome 5. It works correctly in the nightly.