1079151 - Logical coordinate support for absolute positioning

Here are a bunch of examples that set position:absolute but give no explicit offsets, so the positioned element should stay in its original position. We currently fail all the vertical writing-mode examples here. For comparison, Chrome renders this as expected, except for a minor glitch in positioning of the broken-image icon in the vertical-lr cases.

Gérard Talbot

Comment 3

•

10 years ago

Gérard Talbot

Comment 4

•

10 years ago

Other (vendor-prefixed) test failures: http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-icb-vrl-032.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-icb-vlr-033.xht ------ I spent well over an hour on http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-icb-vrl-020.xht and http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-icb-vlr-021.xht trying to figure out why there is an horizontal scrollbar being generated (which then generates a vertical scrollbar) and I just could not find why. I think this issue is related to <object> in 'direction: ltr' when its embedded document's 'direction' is 'rtl'. And so I think this issue has nothing to do per se with writing-mode implementation. There is possibly a bug report already on this... ------ Another (vendor-prefixed) test failure: http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/abs-overconstrained-writing-mode.html The beforementioned test failures happen in Firefox 40.0a1 buildID=20150430100104.

Jonathan Kew [:jfkthame]

Assignee

Comment 5

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0420a4cae20f

Jonathan Kew [:jfkthame]

Assignee

Comment 6

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f4617e9e872

Jonathan Kew [:jfkthame]

Assignee

Comment 7

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=150a07f75c61

Jonathan Kew [:jfkthame]

Assignee

Comment 8

•

10 years ago

Attached patch Tests for position:absolute in vertical writing modes, from Gérard Talbot; currently failing 74 out of 96 testcases (obsolete) — Details — Splinter Review

Here's a patch incorporating the 96 testcases from comment 1, with corresponding reference files so we can run them as reftests. As noted in comment 1, we currently fail 74 out of the 96 cases. (I have a local WIP patch that currently reduces this to 10 failures, so still a bit more to do...)

Attachment #8606977 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Updated

•

10 years ago

Assignee: nobody → jfkthame

Status: NEW → ASSIGNED

Jonathan Kew [:jfkthame]

Assignee

Comment 9

•

10 years ago

Gérard, I'm looking at http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-017.xht, which is one of the tests that still fails for me with local patches ... and I don't think I understand it. AFAICS, the Firefox rendering (in current Nightly, and also in my locally-patched build) is correct; this also agrees with IE11, although it disagrees with Chrome. As far as I can tell, in this testcase the <span>s static position should be immediately after the "34" glyphs, on the same (horizontal) line within the containing <div>, just as if no 'position' or 'writing-mode' were applied to it. I don't know why Chrome moves it below, and the explanatory comments in the file don't appear to match the actual code. Could you please double-check this one? Thanks!

Flags: needinfo?(bugzilla)

Jonathan Kew [:jfkthame]

Assignee

Comment 10

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f31af18ad905

Jonathan Kew [:jfkthame]

Assignee

Comment 11

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a5a30ffc1ea

Jonathan Kew [:jfkthame]

Assignee

Comment 12

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c3422f4df1b

Gérard Talbot

Comment 13

•

10 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #9) > Gérard, I'm looking at > http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non- > replaced-vlr-017.xht, which is one of the tests that still fails for me with > local patches ... and I don't think I understand it. AFAICS, the Firefox > rendering (in current Nightly, and also in my locally-patched build) is > correct; this also agrees with IE11, although it disagrees with Chrome. > > As far as I can tell, in this testcase the <span>s static position should be > immediately after the "34" glyphs, on the same (horizontal) line within the > containing <div>, just as if no 'position' or 'writing-mode' were applied to > it. Yes, agreed. If 'right' is 'auto' in that test, then the <span>s static position should be immediately after the "34" glyphs. There is an error in that s71-abs-pos-non-replaced-vlr-017 test. Doh! <title>CSS Writing Modes Test: absolutely positioned non-replaced element - 'direction: ltr' and 'left' and 'width' are 'auto' and 'right' is not 'auto'</title> (...) line 37 right: auto; but it should be line 37 right: 2em; http://test.csswg.org/source/css-writing-modes-3/abs-pos-non-replaced-vlr-017.xht has the same error! I also think the background-image is wrong too. It should be support/bg-red-2col-2row-320x320.png and the background-image is wrong too for abs-pos-non-replaced-vrl-016.xht as I think it should be also support/bg-red-2col-2row-320x320.png Jonathan, I will fix those 4 tests later this evening. I must go now.... Gérard

Flags: needinfo?(bugzilla)

Gérard Talbot

Comment 14

•

10 years ago

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vrl-016.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-017.xht have been corrected (background-image modified, reference file modified, right: 2em). Firefox 41 and IE11 now pass these 2 tests. The correspondent tests in the test suite have also been modified accordingly: http://hg.csswg.org/test/rev/a224fef2b3ce Jonathan, let me know if you find any other tests that seems incorrect. I will random-review approx. 20 of my own s71-abs-pos-non-replaced-* tests today. Gérard

Gérard Talbot

Comment 15

•

10 years ago

Okay. I just finished re-examining 19 tests: those ending with a "5" and ending with an "8" (eg. s71-abs-pos-non-replaced-vlr-005 s71-abs-pos-non-replaced-vrl-008 s71-abs-pos-non-replaced-vlr-015 s71-abs-pos-non-replaced-vrl-018 s71-abs-pos-non-replaced-vlr-025 s71-abs-pos-non-replaced-vrl-028 ...... ...... s71-abs-pos-non-replaced-vlr-085 s71-abs-pos-non-replaced-vrl-088 s71-abs-pos-non-replaced-vlr-095 ) and I think they are all correct tests. In a few tests, the comments should be updated to reflect the rules given in §10.6.4 instead of §10.3.7 but that's it. -------- In this test http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-065.xht Firefox 41 buildID=20150513191426 Inspector tool reports that the computed 'height' of the <span> is 240px (that does not make sense: it should be based on content which is 80px) and computed 'bottom' of the <span> is 0 (that does not make sense: it should be 160px). -------- Again, Jonathan, if you believe one of my tests is incorrect, let me know. This is appreciated. Gérard

Simon Montagu :smontagu

Reporter

Comment 16

•

10 years ago

Comment on attachment 8606977 [details] [diff] [review] Tests for position:absolute in vertical writing modes, from Gérard Talbot; currently failing 74 out of 96 testcases Review of attachment 8606977 [details] [diff] [review]: ----------------------------------------------------------------- I can rs=me if you like, but why don't you just check these in as author=Gérard r=jfkthame?

Jonathan Kew [:jfkthame]

Assignee

Comment 17

•

10 years ago

(In reply to Simon Montagu :smontagu from comment #16) > Comment on attachment 8606977 [details] [diff] [review] > Tests for position:absolute in vertical writing modes, from Gérard Talbot; > currently failing 74 out of 96 testcases > > Review of attachment 8606977 [details] [diff] [review]: > ----------------------------------------------------------------- > > I can rs=me if you like, but why don't you just check these in as > author=Gérard r=jfkthame? OK, that makes sense. FWIW, note that I generated an alternative set of reference files (to avoid comparing an Ahem glyph with a PNG swatch), though unfortunately this still doesn't eliminate all antialiasing fuzz at the glyph edges, especially on Windows. But the actual test files are directly from Gérard's site. I'll update per comment 14 before checking in, anyway.

Jonathan Kew [:jfkthame]

Assignee

Comment 18

•

10 years ago

(In reply to Gérard Talbot from comment #15) > In this test > > http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non- > replaced-vlr-065.xht > > Firefox 41 buildID=20150513191426 Inspector tool reports that the computed > 'height' of the <span> is 240px (that does not make sense: it should be > based on content which is 80px) and computed 'bottom' of the <span> is 0 > (that does not make sense: it should be 160px). Right, that's a clear Gecko bug. It's fixed in the patches I'm working on, but not quite ready for review & landing yet.

Jonathan Kew [:jfkthame]

Assignee

Comment 19

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dfadb85c653

Jonathan Kew [:jfkthame]

Assignee

Comment 20

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d33cbffeb15

Jonathan Kew [:jfkthame]

Assignee

Comment 21

•

10 years ago

Attached patch Tests for position:absolute in vertical writing modes; currently failing 72 out of 96 testcases — Details — Splinter Review

Updated the reftest patch with the corrected test files, and made the fuzz annotations specific to OS X and Win/D2D only.

Jonathan Kew [:jfkthame]

Assignee

Updated

•

10 years ago

Attachment #8606977 - Attachment is obsolete: true

Attachment #8606977 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 22

•

10 years ago

Comment on attachment 8607627 [details] [diff] [review] Tests for position:absolute in vertical writing modes; currently failing 72 out of 96 testcases Tests written by Gérard Talbot; new reference files and review=jfkthame.

Attachment #8607627 - Flags: review+

Jonathan Kew [:jfkthame]

Assignee

Comment 23

•

10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/111f370d0b43

Jonathan Kew [:jfkthame]

Assignee

Comment 24

•

10 years ago

Marking leave-open, as the landing here is just testcases so far; actual patches still to come...

Keywords: leave-open

Carsten Book [:Tomcat]

Comment 25

•

10 years ago

https://hg.mozilla.org/mozilla-central/rev/111f370d0b43

Jonathan Kew [:jfkthame]

Assignee

Comment 26

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de570f638bac

Jonathan Kew [:jfkthame]

Assignee

Comment 27

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4fd073b4020

Jonathan Kew [:jfkthame]

Assignee

Comment 28

•

10 years ago

Attached patch patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates (obsolete) — Details — Splinter Review

First part of position:absolute is further changes in nsHTMLReflowState; this builds on top of the patches in bug 1147834. I split this from the nsAbsoluteContainingBlock changes (coming next) to make the patches a more reasonable size, but they should be folded together for landing as they're not really independent.

Attachment #8608578 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 29

•

10 years ago

Attached patch patch 2 - Convert nsAbsoluteContainingBlock to use logical coordinates (obsolete) — Details — Splinter Review

Part 2 of the changes. With this, we pass all 96 of the testcases from comment 1, as well as fixing the remaining reftest regressions from bug 1147834's patches. See try run in comment 26, where the oranges are all the unexpected-passes on the abspos testcases.

Attachment #8608580 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 30

•

10 years ago

Attached patch patch 3 - Remove failure annotations from the vertical abs-pos reftests (obsolete) — Details — Splinter Review

Finally, we can remove all those failure annotations.

Attachment #8608581 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 31

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79a0df600d0b

Jonathan Kew [:jfkthame]

Assignee

Comment 32

•

10 years ago

Argh, tryserver says there are still some failures on Android; in the comment 26 try run, there were fewer unexpected-passes than on desktop platforms. :(

Jonathan Kew [:jfkthame]

Assignee

Comment 33

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46f48e60b57f

Jonathan Kew [:jfkthame]

Assignee

Comment 34

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f287da7a84af

Mats Palmgren (inactive)

Updated

•

10 years ago

Depends on: 1167155

Mats Palmgren (inactive)

Updated

•

10 years ago

No longer depends on: 1167155

Jonathan Kew [:jfkthame]

Assignee

Comment 35

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdca41accff1

Gérard Talbot

Comment 36

•

10 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #9) > (...) the explanatory comments in > the file don't appear to match the actual code. (In reply to Gérard Talbot from comment #15) > the comments should be updated to reflect the rules given in > §10.6.4 instead of §10.3.7 I am examining, revisiting, trimming and correcting the comments in all 96 tests right now. Almost all of them had incorrect comments... ---------- This test http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-093.xht is incorrect. "If the values are over-constrained, ignore the value for 'bottom' and solve for that value." §10.6.4 10.6.4 Absolutely positioned, non-replaced elements http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-height Bottom should be ignored and then solved; but the test says top should be ignored and then solved. So, in s71-abs-pos-non-replaced-vlr-093.xht background: red url("support/bg-red-2col-2row-320x320.png") should be instead background: red url("support/bg-red-2col-3row-320x320.png") I must check the other tests...

Gérard Talbot

Comment 37

•

10 years ago

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vrl-092.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-093.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vrl-096.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-097.xht were incorrect because bottom was to be ignored and solved, not top. And so the background-image was wrong and the reference file was wrong for those tests. I will be uploading changes to those 4 tests soon...

Gérard Talbot

Comment 38

•

10 years ago

Gérard Talbot

Comment 39

•

10 years ago

Gérard Talbot

Comment 40

•

10 years ago

IE11 passes all the 12 tests mentioned in #c39 except those (72 and 73).

Gérard Talbot

Comment 41

•

10 years ago

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vrl-092.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-093.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vrl-096.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-097.xht IE11 fails 096 and 097. Chrome 44.0.2403.9 and Firefox 41.0a1 buildID=20150513191426 fail all these 4 tests. But I'm convinced those 4 uploaded tests are correct. ------------ (In reply to Jonathan Kew (:jfkthame) from comment #29) > we pass all 96 of the testcases from > comment 1 I think you need to check your patches against those 4 tests and the 12 tests in #c39 .

Jonathan Kew [:jfkthame]

Assignee

Comment 42

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ac0d03f94e

Jonathan Kew [:jfkthame]

Assignee

Comment 43

•

10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7837feefe648

Gérard Talbot

Comment 44

•

9 years ago

I had a talk with fantasai this evening and it appears that I did not understand the following sentence: " Layout rules that refer to the *-left and *-right box properties (border, margin, padding, positioning offsets) use *-top and *-bottom instead, and vice versa, mapping the horizontal writing-mode rules of CSS2.1 into vertical writing-mode rules using the flow-relative directions. " therefore I will have to reexamine and reassess (and most likely modify some of) those 96 tests again, probably this week. I am extremely sorry about this unexpected turn of events. I really want these tests (including their comments) to be correct, to be reliable, to be trustworthy.

Jonathan Kew [:jfkthame]

Assignee

Comment 45

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a7a217b7f5d

Jonathan Kew [:jfkthame]

Assignee

Comment 46

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bd3d69d2ba5

Jonathan Kew [:jfkthame]

Assignee

Comment 47

•

9 years ago

Attached patch patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates (obsolete) — Details — Splinter Review

Attachment #8610238 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Attachment #8608578 - Attachment is obsolete: true

Attachment #8608578 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 48

•

9 years ago

Attached patch patch 2 - Convert nsAbsoluteContainingBlock to use logical coordinates — Details — Splinter Review

Attachment #8610239 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Attachment #8608580 - Attachment is obsolete: true

Attachment #8608580 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 49

•

9 years ago

Updated on top of the current patches in bug 1167930 and bug 1147834. Tryserver (see comment 45) says that this fixes all unit test regressions (reftest failures, assertions) remaining from bug 1147834 by itself.

Jonathan Kew [:jfkthame]

Assignee

Comment 50

•

9 years ago

Attached file testcase for positioning property experiments — Details

Here's an updated version of attachment 8599240 [details] that I've been using to experiment interactively with various combinations modes and properties. With the latest patches here, this seems to work well.

Attachment #8599240 - Attachment is obsolete: true

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Depends on: 1167930

Jonathan Kew [:jfkthame]

Assignee

Comment 51

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=583c441bb8cc

Simon Montagu :smontagu

Reporter

Updated

•

9 years ago

Attachment #8608581 - Flags: review?(smontagu) → review+

Simon Montagu :smontagu

Reporter

Comment 52

•

9 years ago

Comment on attachment 8610238 [details] [diff] [review] patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates Review of attachment 8610238 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsHTMLReflowState.cpp @@ +1024,5 @@ > > // When determining the hypothetical box that would have been if the element > +// had been in the flow we may not be able to exactly determine both the IStart > +// and IEnd edges. For example, if the element is a non-replaced inline-level > +// element we would have to reflow it in order to determine it desired ISize. s/it/its/ (the second one on the line) while you're here @@ -1337,2 @@ > #ifdef DEBUG > - aHypotheticalBox.mRightIsExact = true; AFAICT you still need to set mIEndIsExact to true where appropriate @@ +1498,3 @@ > } > + if (eStyleUnit_Auto == mStylePosition->mOffset.GetBEndUnit(cbwm)) { > + offsets.BEnd(cbwm) = 0; Nit: trailing whitespace not removed

Attachment #8610238 - Flags: review?(smontagu) → review+

Simon Montagu :smontagu

Reporter

Updated

•

9 years ago

Attachment #8610239 - Flags: review?(smontagu) → review+

Jonathan Kew [:jfkthame]

Assignee

Comment 53

•

9 years ago

One more issue here: tryserver shows that the vertical-rl testcases fail intermittently (but frequently) on all Android versions. I can reproduce this locally, too. The failures vary from one run to another, even on the exact same build -- although the nature of the failure is always the same, with the abspos element appearing substantially to the right of its correct position -- so I think we need to mark all the vrl testcases as "random" for now. I'll file a followup bug on this.

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Depends on: 1169311

Jonathan Kew [:jfkthame]

Assignee

Comment 54

•

9 years ago

Attached patch patch 4 - Mark vertical-rl testcases as random-if(Android) due to intermittent failures (obsolete) — Details — Splinter Review

Attachment #8612349 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 55

•

9 years ago

Comment on attachment 8612349 [details] [diff] [review] patch 4 - Mark vertical-rl testcases as random-if(Android) due to intermittent failures Cancelling this; the erratic behavior on Android reftests is a result of a bug in patch 1 here, and will be fixed by a corrected version.

Attachment #8612349 - Attachment is obsolete: true

Attachment #8612349 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 56

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=938fd196b3ee

Jonathan Kew [:jfkthame]

Assignee

Comment 57

•

9 years ago

Attached patch patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates (obsolete) — Details — Splinter Review

Updated part 1 to fix the Android reftest failures etc, so we won't need the followup bug 1169311; better to get it right in the initial landing. :) Carrying forward r=smontagu.

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Attachment #8610238 - Attachment is obsolete: true

Jonathan Kew [:jfkthame]

Assignee

Comment 58

•

9 years ago

Gérard, I've been looking at the tests mentioned in comment 41, and am not convinced they are correct. Taking s71-abs-pos-non-replaced-vlr-093.xht, we have div#containing-block > span { background-color: red; bottom: 2em; color: green; height: 1em; margin-bottom: 0em; margin-top: 0em; position: absolute; top: 2em; } and the containing block has a height of 4em. So the span is overconstrained, and either the |top| or |bottom| value will have to be discarded and computed from |height| and the other offset. Your testcase expects |top| to be kept, and |bottom| solved (resulting in 1em), but because in this testcase we have direction:rtl (inherited from the containing block), I think it should be the other way around: the specified |bottom| should be used, and |top| computed to 1em. The same applies to tests 92, 96 and 97, AFAICS. Please double-check, and let me know if I'm misunderstanding here. Thanks again!

Flags: needinfo?(bugzilla)

Gérard Talbot

Comment 59

•

9 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #58) > Gérard, I've been looking at the tests mentioned in comment 41, and am not > convinced they are correct. And your belief is right, is correct. > Taking s71-abs-pos-non-replaced-vlr-093.xht, we > have > > div#containing-block > span > { > background-color: red; > bottom: 2em; > color: green; > height: 1em; > margin-bottom: 0em; > margin-top: 0em; > position: absolute; > top: 2em; > } > > and the containing block has a height of 4em. So the span is > overconstrained, and either the |top| or |bottom| value will have to be > discarded and computed from |height| and the other offset. > > Your testcase expects |top| to be kept, and |bottom| solved (resulting in > 1em), but because in this testcase we have direction:rtl (inherited from the > containing block), I think it should be the other way around: the specified > |bottom| should be used, and |top| computed to 1em. The specified 'bottom' should be kept and then 'top' should be ignored and then solved, because 'direction' is 'rtl'. In comment #44, I explained my confusion, misunderstanding. I agree with your explanation. > The same applies to tests 92, 96 and 97, AFAICS. Please double-check, and > let me know if I'm misunderstanding here. Thanks again! The thing is: the initial version of those tests were probably correct. I've been sick all week long and had some other urgent task to do this week. Re-visiting and reviewing all 96 abs-pos-non-replaced-v* tests is on my top priority to-do-list right now; I want the /* comments */ to be clear, reliable and trustworthy too. Gérard

Flags: needinfo?(bugzilla)

Gérard Talbot

Comment 60

•

9 years ago

Jonathan Kew [:jfkthame]

Assignee

Comment 61

•

9 years ago

Comment on attachment 8612827 [details] [diff] [review] patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates Review of attachment 8612827 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsHTMLReflowState.cpp @@ +1602,5 @@ > // * we're dealing with a replaced element > // * width was constrained by min-width or max-width. > > + nscoord availMarginSpace = > + aCBSize.Width(cbwm) - offsets.IStartEnd(cbwm) - margin.IStartEnd(cbwm) - Oops... s/Width/ISize/ here. Corrected patch coming shortly.

Jonathan Kew [:jfkthame]

Assignee

Comment 62

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bc74ae26a1f

Jonathan Kew [:jfkthame]

Assignee

Comment 63

•

9 years ago

Attached patch Updated test files with corrected comments and references as needed — Details — Splinter Review

This updates the test files to the current versions from Gérard's site. We still fail most of them, until the patches here (and dependencies) actually land.

Jonathan Kew [:jfkthame]

Assignee

Comment 64

•

9 years ago

Attached patch patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates — Details — Splinter Review

Updated to fix the error noted in comment 61.

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Attachment #8612827 - Attachment is obsolete: true

Jonathan Kew [:jfkthame]

Assignee

Comment 65

•

9 years ago

Attached patch patch 3 - Remove failure annotations from the vertical abs-pos reftests (obsolete) — Details — Splinter Review

Updated patch 3 to match the corrected testcases; carrying over r=smontagu.

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Attachment #8608581 - Attachment is obsolete: true

Jonathan Kew [:jfkthame]

Assignee

Comment 66

•

9 years ago

With the patches above, and Gérard's corrected testcases, we are left with 4 remaining failures: s71-abs-pos-non-replaced-vlr-065.xht s71-abs-pos-non-replaced-vlr-071.xht s71-abs-pos-non-replaced-vrl-064.xht s71-abs-pos-non-replaced-vrl-070.xht These have specified 'left' and 'right' offsets, and width:auto; but the test block is orthogonal to its absolute containing block, and so its block-size (which is unconstrained when we call ComputedSize on it) becomes inline-size when we're working in the containing block's writing mode, and we don't handle unconstrained inline-size well. The net result is that the test block, which should end up 1em square, collapses to zero in its block direction (the container's inline direction) due to an NS_UNCONSTRAINEDSIZE in the computations in nsHTMLReflowState::InitAbsoluteConstraints.

Jonathan Kew [:jfkthame]

Assignee

Comment 67

•

9 years ago

Attached file testcase showing issues with 'auto' inline-size on orthogonal abs-pos blocks — Details

Here are a set of testcases to illustrate the issue above. Current trunk code renders only 4 of these cases correctly, and 8 fail. With patches 1 and 2 above, we pass 6, but still fail 6 of the cases.

Jonathan Kew [:jfkthame]

Assignee

Comment 68

•

9 years ago

Attached patch patch 4 - Handle unconstrained inline-size when computing constraints for an orthogonal absolutely-positioned block (obsolete) — Details — Splinter Review

And here's the patch that makes all 12 examples in attachment 8613299 [details] render correctly, as well as fixing the 4 remaining failures in Gérard's test collection.

Attachment #8613300 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Comment 69

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=830aefe0eb7e

Jonathan Kew [:jfkthame]

Assignee

Comment 70

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4902b1ada67a

Jonathan Kew [:jfkthame]

Assignee

Comment 71

•

9 years ago

Attached patch patch 3 - Remove failure annotations from the vertical abs-pos reftests — Details — Splinter Review

Now with adjusted fuzz to keep Windows tests happy.

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Attachment #8613298 - Attachment is obsolete: true

Jonathan Kew [:jfkthame]

Assignee

Comment 72

•

9 years ago

Attached patch patch 4 - Handle unconstrained inline-size when computing constraints for an orthogonal absolutely-positioned block (obsolete) — Details — Splinter Review

With updated fuzz annotations.

Attachment #8613317 - Flags: review?(smontagu)

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Attachment #8613300 - Attachment is obsolete: true

Attachment #8613300 - Flags: review?(smontagu)

Gérard Talbot

Comment 73

•

9 years ago

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vlr-023.xht says line 7: <title> ... and 'right' is not 'auto'</title> ... line 37: right: auto; The error is also in http://test.csswg.org/source/css-writing-modes-3/abs-pos-non-replaced-vlr-023.xht The background-image and the comments appear correct regardless if 'right' is '2em' or if 'right' is 'auto' in that test. I will be fixing this test this evening.

Simon Montagu :smontagu

Reporter

Comment 74

•

9 years ago

Comment on attachment 8613317 [details] [diff] [review] patch 4 - Handle unconstrained inline-size when computing constraints for an orthogonal absolutely-positioned block Review of attachment 8613317 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsHTMLReflowState.cpp @@ +1614,5 @@ > + > + // XXX Do these need box-sizing adjustments? > + LogicalSize maxSize = > + LogicalSize(wm, ComputedMaxISize(), > + ComputedMaxBSize()).ConvertTo(cbwm, wm); LogicalSize maxSize = ComputedMaxSize(cbwm); LogicalSize minSize = ComputedMinSize(cbwm);

Attachment #8613317 - Flags: review?(smontagu) → review+

Jonathan Kew [:jfkthame]

Assignee

Comment 75

•

9 years ago

Comment on attachment 8613317 [details] [diff] [review] patch 4 - Handle unconstrained inline-size when computing constraints for an orthogonal absolutely-positioned block Review of attachment 8613317 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsHTMLReflowState.cpp @@ +1614,5 @@ > + > + // XXX Do these need box-sizing adjustments? > + LogicalSize maxSize = > + LogicalSize(wm, ComputedMaxISize(), > + ComputedMaxBSize()).ConvertTo(cbwm, wm); Good point, and I'll do the same to the parallel block-coordinate code.

Jonathan Kew [:jfkthame]

Assignee

Comment 76

•

9 years ago

Attached patch patch 4 - Handle unconstrained inline-size when computing constraints for an orthogonal absolutely-positioned block — Details — Splinter Review

Updated per comment 74, carrying over r=smontagu.

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Attachment #8613317 - Attachment is obsolete: true

Jonathan Kew [:jfkthame]

Assignee

Comment 77

•

9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/780a3290c0ed

Jonathan Kew [:jfkthame]

Assignee

Comment 78

•

9 years ago

^^ pushed updated/corrected testcases from Gérard; keeping the bug open ready for the actual patches to land.

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Depends on: 1147834

Gérard Talbot

Comment 79

•

9 years ago

http://hg.csswg.org/test/rev/9f35de3f1ab3 ( made, submitted late on may 31st ) corresponds to the revised, corrected-comment tests in the Writing Modes test suite. I most likely will need to create another batch of 96 tests checking, verifying that layout calculation rules (§10.6.4) that apply to the vertical dimension (in block axis) in horizontal writing modes instead apply to the horizontal dimension (block axis) in vertical writing modes. Because there are no test right now checking this! The added difficulty to take into consideration (when creating those tests) is that - block-start in vertical-RL is on the righthand side; so 'top' in §10.6.4 rules maps to 'right' in vRL - block-start in vertical-LR is on the lefthand side; so 'top' in §10.6.4 rules maps to 'left' in vLR - block-end in vertical-RL is on the lefthand side; so 'bottom' in §10.6.4 rules maps to 'left' in vRL - block-end in vertical-LR is on the righthand side; so 'bottom' in §10.6.4 rules maps to 'right' in vLR Gérard

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 80

•

9 years ago

https://hg.mozilla.org/mozilla-central/rev/780a3290c0ed

Jonathan Kew [:jfkthame]

Assignee

Comment 81

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d339e0c7edcc

Jonathan Kew [:jfkthame]

Assignee

Comment 82

•

9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e641f522e5ae https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf49050d29e https://hg.mozilla.org/integration/mozilla-inbound/rev/94fa0e37c525 https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb359c682f0

Jonathan Kew [:jfkthame]

Assignee

Updated

•

9 years ago

Keywords: leave-open

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 84

•

9 years ago

https://hg.mozilla.org/mozilla-central/rev/e641f522e5ae https://hg.mozilla.org/mozilla-central/rev/ddf49050d29e https://hg.mozilla.org/mozilla-central/rev/94fa0e37c525 https://hg.mozilla.org/mozilla-central/rev/5eb359c682f0

Status: ASSIGNED → RESOLVED

Closed: 9 years ago

status-firefox41: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla41

Jean-Yves Perrier [:teoli]

Updated

•

9 years ago

Keywords: dev-doc-needed

Jean-Yves Perrier [:teoli]

Updated

•

9 years ago

Keywords: dev-doc-needed

Phil Ringnalda (:philor)

Updated

•

9 years ago

Depends on: 1174379, 1172376, 1171767, 1169165, 1167155

Gérard Talbot

Comment 85

•

9 years ago

(In reply to Gérard Talbot from comment #79) > I most likely will need to create another batch of 96 tests checking, > verifying that layout calculation rules (§10.6.4) that apply to the vertical > dimension (in block axis) in horizontal writing modes instead apply to the > horizontal dimension (block axis) in vertical writing modes. Because there > are no test right now checking this! The added difficulty to take into > consideration (when creating those tests) is that > > - block-start in vertical-RL is on the righthand side; so 'top' in §10.6.4 > rules maps to 'right' in vRL > > - block-start in vertical-LR is on the lefthand side; so 'top' in §10.6.4 > rules maps to 'left' in vLR > > - block-end in vertical-RL is on the lefthand side; so 'bottom' in §10.6.4 > rules maps to 'left' in vRL > > - block-end in vertical-LR is on the righthand side; so 'bottom' in §10.6.4 > rules maps to 'right' in vLR Just for your information. Another batch of 128 tests verifying that layout calculation rules (§10.6.4) have been created and have been submitted: with writing-mode: vertical-lr - - - - - - - - - - - - - - - http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/abs-pos-non-replaced-vlr-103.htm http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/abs-pos-non-replaced-vlr-105.htm ... http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/abs-pos-non-replaced-vlr-227.htm http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/abs-pos-non-replaced-vlr-229.htm with writing-mode: vertical-rl - - - - - - - - - - - - - - - http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/abs-pos-non-replaced-vrl-102.htm http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/abs-pos-non-replaced-vrl-104.htm ... http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/abs-pos-non-replaced-vrl-226.htm http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/abs-pos-non-replaced-vrl-228.htm and Firefox 42.0a1's test results have been gathered too: http://test.csswg.org/harness/results/css-writing-modes-3_dev/grouped/browser/Firefox/browser_version/42.0/#s7.1 Gérard

Daniel Holbert [:dholbert]

Updated

•

9 years ago

Blocks: 1191185

Gérard Talbot

Comment 86

•

9 years ago

(In reply to Gérard Talbot from comment #4) > I spent well over an hour on > > http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non- > replaced-icb-vrl-020.xht > > and > > http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non- > replaced-icb-vlr-021.xht > > trying to figure out why there is an horizontal scrollbar being generated > (which then generates a vertical scrollbar) and I just could not find why. I > think this issue is related to <object> in 'direction: ltr' when its > embedded document's 'direction' is 'rtl'. And so I think this issue has > nothing to do per se with writing-mode implementation. There is possibly a > bug report already on this... Bug 1167911 has been created specifically for those 2 tests.

testcases for position:absolute with various writing modes 10 years ago Jonathan Kew [:jfkthame] 2.12 KB, text/html		Details
Tests for position:absolute in vertical writing modes, from Gérard Talbot; currently failing 74 out of 96 testcases 10 years ago Jonathan Kew [:jfkthame] 514.95 KB, patch		Details \| Diff \| Splinter Review
Tests for position:absolute in vertical writing modes; currently failing 72 out of 96 testcases 10 years ago Jonathan Kew [:jfkthame] 515.84 KB, patch	jfkthame : review+	Details \| Diff \| Splinter Review
patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates 10 years ago Jonathan Kew [:jfkthame] 48.82 KB, patch		Details \| Diff \| Splinter Review
patch 2 - Convert nsAbsoluteContainingBlock to use logical coordinates 10 years ago Jonathan Kew [:jfkthame] 20.29 KB, patch		Details \| Diff \| Splinter Review
patch 3 - Remove failure annotations from the vertical abs-pos reftests 10 years ago Jonathan Kew [:jfkthame] 19.31 KB, patch	smontagu : review+	Details \| Diff \| Splinter Review
patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates 9 years ago Jonathan Kew [:jfkthame] 47.31 KB, patch	smontagu : review+	Details \| Diff \| Splinter Review
patch 2 - Convert nsAbsoluteContainingBlock to use logical coordinates 9 years ago Jonathan Kew [:jfkthame] 20.12 KB, patch	smontagu : review+	Details \| Diff \| Splinter Review
testcase for positioning property experiments 9 years ago Jonathan Kew [:jfkthame] 6.52 KB, text/html		Details
patch 4 - Mark vertical-rl testcases as random-if(Android) due to intermittent failures 9 years ago Jonathan Kew [:jfkthame] 14.88 KB, patch		Details \| Diff \| Splinter Review
patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates 9 years ago Jonathan Kew [:jfkthame] 47.35 KB, patch		Details \| Diff \| Splinter Review
Updated test files with corrected comments and references as needed 9 years ago Jonathan Kew [:jfkthame] 299.00 KB, patch		Details \| Diff \| Splinter Review
patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates 9 years ago Jonathan Kew [:jfkthame] 47.35 KB, patch		Details \| Diff \| Splinter Review
patch 3 - Remove failure annotations from the vertical abs-pos reftests 9 years ago Jonathan Kew [:jfkthame] 18.83 KB, patch		Details \| Diff \| Splinter Review
testcase showing issues with 'auto' inline-size on orthogonal abs-pos blocks 9 years ago Jonathan Kew [:jfkthame] 1.56 KB, text/html		Details
patch 4 - Handle unconstrained inline-size when computing constraints for an orthogonal absolutely-positioned block 9 years ago Jonathan Kew [:jfkthame] 8.79 KB, patch		Details \| Diff \| Splinter Review
patch 3 - Remove failure annotations from the vertical abs-pos reftests 9 years ago Jonathan Kew [:jfkthame] 18.88 KB, patch		Details \| Diff \| Splinter Review
patch 4 - Handle unconstrained inline-size when computing constraints for an orthogonal absolutely-positioned block 9 years ago Jonathan Kew [:jfkthame] 8.87 KB, patch	smontagu : review+	Details \| Diff \| Splinter Review
patch 4 - Handle unconstrained inline-size when computing constraints for an orthogonal absolutely-positioned block 9 years ago Jonathan Kew [:jfkthame] 9.66 KB, patch		Details \| Diff \| Splinter Review