Closed
Bug 410229
Opened 17 years ago
Closed 17 years ago
getBoundingClientRect/getClientRects do not process block-in-inline "special siblings"
Categories
(Core :: DOM: CSS Object Model, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta3
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(3 files)
5.47 KB,
patch
|
MatsPalmgren_bugz
:
review+
MatsPalmgren_bugz
:
superreview+
|
Details | Diff | Splinter Review |
4.56 KB,
text/html
|
Details | |
3.88 KB,
patch
|
MatsPalmgren_bugz
:
review+
MatsPalmgren_bugz
:
superreview+
|
Details | Diff | Splinter Review |
See attached testcase. We're not looking at special siblings, so the rectangle(s) returned only include the first part of the IB split. This will be easy to fix and we should fix it because this is a new feature and we don't want it to be broken for the Web.
Flags: blocking1.9?
Comment 1•17 years ago
|
||
(In reply to comment #0) > See attached testcase. I think Bugzilla ate your attachment (this happened to me recently) :-(
Assignee | ||
Comment 2•17 years ago
|
||
This fixes the problem with liberal use of GetNextContinuationOrSpecialSibling, and adds a mochitest. I'm fixing GetAllInFlowBoundingRect because these DOM APIs use it. This will also affect offset* properties, but that's OK --- this bug applies to them too.
Attachment #295185 -
Flags: superreview?(mats.palmgren)
Attachment #295185 -
Flags: review?(mats.palmgren)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs review]
Comment 3•17 years ago
|
||
Comment 4•17 years ago
|
||
Comment on attachment 295185 [details] [diff] [review] fix > nsIFrame* next; >- while ((next = frame->GetNextContinuation()) != nsnull) { I think this loop would be clearer if we remove 'next', can we just assign 'frame' directly instead? > NS_ASSERTION(isSVG, "SVG frames must have SVG continuations"); Does this still hold? What about a html:div in a SVG inline, wouldn't that create a special sibling where 'isSVG' is false? r+sr=mats with the above addressed as you see fit, but please also consider adding some of the additional tests I attached above. Please also verify that those test results are the desired results, specifically for the empty rects - should we have these at all and should they be outside the parent's bounding rect? and in the last case - should we only have two rects? Just so I know - we are trying to emulate IE here I suppose?
Attachment #295185 -
Flags: superreview?(mats.palmgren)
Attachment #295185 -
Flags: superreview+
Attachment #295185 -
Flags: review?(mats.palmgren)
Attachment #295185 -
Flags: review+
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Assignee | ||
Comment 5•17 years ago
|
||
> can we just assign 'frame' directly instead? Yeah I guess so! > Does this still hold? What about a html:div in a SVG inline, wouldn't > that create a special sibling where 'isSVG' is false? We don't have real SVG inline frames. > Just so I know - we are trying to emulate IE here I suppose? No. This was just a bug I realized we had. I'll check in your tests, thanks. Those zero width rects seem a little strange but I guess that's where the left and right border of the span would go if it had them, so I guess they're logical. It is correct per spec for empty rects to not contribute to the bounding rect.
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Assignee | ||
Comment 6•17 years ago
|
||
Two of the tests failed on Mac. This patch fixes and reenables them. I was just scaling/rounding the rect badly.
Attachment #295323 -
Flags: superreview?(mats.palmgren)
Attachment #295323 -
Flags: review?(mats.palmgren)
Comment 7•17 years ago
|
||
Comment on attachment 295323 [details] [diff] [review] fix test failures r+sr=mats
Attachment #295323 -
Flags: superreview?(mats.palmgren)
Attachment #295323 -
Flags: superreview+
Attachment #295323 -
Flags: review?(mats.palmgren)
Attachment #295323 -
Flags: review+
Updated•17 years ago
|
Target Milestone: --- → mozilla1.9 M11
Assignee | ||
Comment 8•17 years ago
|
||
I already checked that patch in to fix Tinderbox orange.
Depends on: 417255
You need to log in
before you can comment on or make changes to this bug.
Description
•