Closed Bug 898797 Opened 12 years ago Closed 12 years ago

Use ApplyRelativePositioning when placing floats

Categories

(Core :: Layout: Positioned, defect)

Product:
Core
Component:
Layout: Positioned
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: coyotebush, Assigned: coyotebush)

References

Details

Attachments

(2 files, 1 obsolete file)

Reftests for relatively positioned floats. (v2)
6.07 KB, patch
coyotebush
: review+
Details | Diff | Splinter Review
Use nsHTMLReflowState::ApplyRelativePositioning when placing floats. r=
6.47 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
https://hg.mozilla.org/mozilla-central/file/75e2498668df/layout/generic/nsBlockReflowState.cpp#l820 is one case I arguably missed in bug 893962. This should use nsHTMLReflowState::ApplyRelativePositioning, though I'm not actually sure where to find the computed offsets if not in the frame property (which bug 898794 would otherwise eliminate). It doesn't look like mReflowState would contain the right ones at this point in the code. It's also worth considering what we want the behavior of sticky positioned floats to be. I'd guess it should be identical to position:relative (so it sticks, but doesn't re-wrap while scrolling), and I assume that's what I would get by making this change. For what it's worth, though, WebKit appears to ignore the position:sticky declaration on floats.
This turns out to be the only use of ComputedOffsetProperty/GetRelativeOffset that I'm not sure how I can get rid of.
Blocks: 898794
Before I touch that line, how about a couple reftests to at least make sure relative positioning of floats works (nothing fancy)? These break if I comment out that line.
Attachment #782886 - Flags: review?(dholbert)
Comment on attachment 782886 [details] [diff] [review] Reftests for relatively positioned floats. >+++ b/layout/reftests/floats/relative-float-1-noref.html >@@ -0,0 +1,2 @@ >+<!DOCTYPE html> >+<div style="float: left; position: relative; height: 100px; width: 100px; background: blue"></div> I'm not sure how much this "-noref" file adds; the "float:left; position:relative" don't actually have any effect, so it's basically a test that the reference case's relpos div doesn't match this non-positioned div. And I'd wager we have tests that cover that already, so I don't think it's necessary to test it here. If you'd strongly like to, I won't object; but in general, I tend to avoid "-noref" files because: (a) it's hard to predict what sorts of "norefs" would actually be useful for catching bugs. (b) the things tested in a -noref are generally better tested directly (i.e. in a "does relative positioning of a div work at all?" test), and they usually already are tested elsewhere. (c) Given (a) and (b), I don't think the benefit outweighs the cost of running the test on every supported platform manymany times per day from now until forever. >diff --git a/layout/reftests/floats/relative-float-1-ref.html b/layout/reftests/floats/relative-float-1-ref.html >new file mode 100644 >--- /dev/null >+++ b/layout/reftests/floats/relative-float-1-ref.html >@@ -0,0 +1,2 @@ >+<!DOCTYPE html> >+<div style="position: relative; top: 20px; left: 20px; height: 100px; width: 100px; background: blue"></div> This applies to all of the files in this patch: - Per IRC discussion, I'd prefer that chunk of style in a <style> block, with each declaration on its own line, so that the testcase and reference case are easier to compare with 'diff' or a merge tool. (to see what we're testing) - Please add the public domain header from the bottom of https://www.mozilla.org/MPL/headers/ (not the MPL header) to the testcases & reference cases, so that other folks can repurpose these tests if they like. r=me with that. Thanks for finding this relatively-untested codepath and adding tests for it!
Attachment #782886 - Flags: review?(dholbert) → review+
Addressed those comments.
Attachment #782886 - Attachment is obsolete: true
Attachment #782900 - Flags: review+
Ah, I can pull the offsets out of nsBlockFrame::ReflowFloat. This passes my new tests just fine.
Attachment #782952 - Flags: review?(dbaron)
Comment on attachment 782952 [details] [diff] [review] Use nsHTMLReflowState::ApplyRelativePositioning when placing floats. r= r=dbaron
Attachment #782952 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07d92b28f721 https://hg.mozilla.org/mozilla-central/rev/b3b6963f6cec
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: