David Baron :dbaron: Reporter
	Description • 9 years ago

There are various differences between the style hints we produce for changes in width and min/max-width compared to height and min/max-height.  These will produce incorrect results for vertical writing-modes.

See nsStylePosition::CalcDifference (including some recent changes there; bug 1172239 comment 42 led me to file this).

I think the most straightforward possible solution is to actually switch what we store from being physical (height/width) to logical (block-size/inline-size).  This, however, presumes that this will produce correct results at the boundaries (i.e., where we switch between vertical and horizontal), which needs to be checked carefully before we assume it will work.

	Jonathan Kew [:jfkthame] Assignee
	Comment 1 • 9 years ago

Yes, there's clearly some incorrect code in nsStylePosition::CalcDifference at the moment. However, I'm not sure switching our storage (in nsStylePosition, I presume you mean) over to logical values will be an overall win. At first glance, it's tempting, but I think it'll lead us into added complexity that we don't really need.

The trouble is that sometimes we'll want the dimensions such as ISize/BSize from a style context in terms of a different writing mode than they were stored as. The two usual cases are that a frame or reflow-state wants these dimensions in its own writing mode, or in its parent's or containing-block's mode.

Currently, we can always get a logical-coord dimension from an nsStylePosition by simply passing the writing mode in which we want to retrieve the value, and letting the accessor convert from the stored physical values to the desired logical one. But if we start storing the values in logical terms, we'll need to either store a writing mode in nsStylePosition as well (so that it knows when it needs to convert to what the caller wants), or ensure that every caller knows how the values were stored in that particular context and deals with mismatches (e.g. passing both stored and desired modes to accessors that handle conversion).

Storing the values physically avoids this extra burden on either the nsStylePosition struct or its callers. I think it's going to be simpler to address this by passing a writing mode to nsStylePosition::CalcDifference so that it can handle the physically-stored values properly when computing (logical) style change hints.

	Jonathan Kew [:jfkthame] Assignee
	Comment 2 • 9 years ago

This isn't very elegant, but I didn't want to burden all the other CalcDifference methods with an extra parameter, nor duplicate the bulk of DO_STRUCT_DIFFERENCE; hence the macro hackery. I don't know whether we always handle these hints adequately across orthogonal-flow boundaries, etc. (probably not, at present), but this should at least enable CalcDifference to start doing the right thing. Does this seem a reasonable approach?

	Jonathan Kew [:jfkthame] Assignee
	Comment 3 • 9 years ago

BTW, do you have examples that will exhibit incorrect behavior because of the mishandling of width/height differences here? I experimented a little bit, but so far only came up with a testcase that fails in Chrome in what looks like a similar kind of way, but it works OK in Firefox (with or without the patch above).

	David Baron :dbaron: Reporter
	Comment 4 • 9 years ago

Comment on attachment 8640362 [details] [diff] [review]
Pass the style context to nsStylePosition::CalcDifference so that it can take account of writing-mode where necessary

Yeah, I'm ok with this.  It is ugly, though.

(Just please be sure you're looking at the correct writing mode between context and parent.)

	Jonathan Kew [:jfkthame] Assignee
	Comment 9 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 10 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 11 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 12 • 9 years ago

Thanks for posting the testcases. I've turned these into reftests and confirmed that (as expected) they fail with current trunk code (see try job in comment 9, which has them marked as expected failures in the manifest).

	Jonathan Kew [:jfkthame] Assignee
	Comment 13 • 9 years ago

Updated patch to apply to current trunk (in particular, following changes in bug 1190635). This makes the tests pass locally; try job in progress, comment 11.

	David Baron :dbaron: Reporter
	Comment 14 • 9 years ago

Comment on attachment 8657608 [details] [diff] [review]
Pass the style context to nsStylePosition::CalcDifference so that it can take account of writing-mode where necessary

Instead of duplicating the code in CalcDifference, it might be nice to compute three booleans, heightChanged, widthChanged, and isVertical, and then just replace the existing conditions with:

  if (isVertical ? widthChanged : heightChanged)

and:

  if (isVertical ? heightChanged : widthChanged)

(This also avoids reindenting  or moving the existing code.)

r=dbaron, preferably with that change

	Jonathan Kew [:jfkthame] Assignee
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2739625b99bf100ec26ab52f373ebf7fd156f78f
Bug 1188061 - (Testcases from dbaron) Reftests for nsChangeHint_* flags that relate to resizing in one dimension only. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef7c7277bd66903738a7011f78bfe16bb040739
Bug 1188061 - Pass the style context to nsStylePosition::CalcDifference so that it can take account of writing-mode where necessary. r=dbaron

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/2739625b99bf
https://hg.mozilla.org/mozilla-central/rev/3ef7c7277bd6

	David Baron :dbaron: Reporter
	Comment 17 • 9 years ago

Thanks for getting this in, and sorry for the long delay on the feedback.