Test ---- http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/background-position-vrl-008.htm Step ---- Horizontally-only resize the window: widen the window and/or narrow the window Expected -------- An unclipped cat image should start exactly inside the bottom right corner of the page. Background-image has to be repainted and repositioned when the window is widened or is narrowed. Explanations ------------ Form controls elements are also not reflowed. There is no problem if/when vertically resizing the window. There is no problem if/when bottom-right corner resizing the window. The problem is only if/when horizontally resizing the window. Notes ----- - Another and better test is coming - Chrome 50.0.2661.94 and Chrome 52.0.2729.3 pass this test - I am using Firefox 49.0a1 buildID=20160515030241 - I use Linux 3.13.0-86-generic x86_64, Qt: 4.8.6, KDE 4.13.3; Kubuntu (trusty) 14.04.4 LTS - I've searched for duplicates and did not find any.

Better test: http://www.gtalbot.org/BugzillaSection/Bug1272997-horizontal-resizing-VRL.html When horizontally-only resizing the window in that test, resizing events are fired but background-image is not repainted and form controls are not reflowed accordingly, while horizontally resizing and once horizontally resizing has stopped. Clicking the Reset button of the form (after horizontally resizing the page) unexpectedly reflows the page but does not reset form controls. It requires a 2nd click of the Reset button to clear the form. So, this should be considered to be a 3rd actual vs expected issue in this bug report.

Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d9c8c1d14037&tochange=52ee1ad644d1 Suspect: 2f789a4def0a Jonathan Kew — Bug 1174507 - Replace the frame flag CONTAINS_RELATIVE_HEIGHT with CONTAINS_RELATIVE_BSIZE, and adjust callsites appropriately. r=dholbert

This bug also affects Firefox 46 buildID=20160425115534 . Another noticeable layout issue is that 1 vertical border of text inputs is intermittently repainted when I bottom-right corner-resize the page. Since the border for text input is some kind of gradient-linear light gray (#eff0f1) color, this may not be quickly or easily noticeable at first.

> when I bottom-right corner-resize the page. It's not just the bottom-right; resizing the page from any of the 4 corners of the window is going to repaint intermittently 1 vertical light gray border of text inputs.

> 1 vertical border of text inputs is > intermittently repainted when I bottom-right corner-resize the page. Since > the border for text input is some kind of gradient-linear light gray > (#eff0f1) color, this may not be quickly or easily noticeable at first. This can also be noticed in other form input controls (<select> and <textarea>) in other tests as affecting both vertical borders: http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-form-controls-vlr-005.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-form-controls-vrl-005.xht http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-form-controls-slr-005.xht

I will create a separate, distinct bug report for the painting of vertical light-gray (#eff0f1) borders of form input controls (<input type="text">, <select>, <textarea>) when window resizing occurs.

Review commit: https://reviewboard.mozilla.org/r/52700/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52700/

The patch above seems to make sense, and may ultimately be needed, but I think there's a more fundamental problem that we should solve first (and then see whether there are remaining issues that need to be fixed here). Consider this testcase, which uses vertical-lr (not -rl) mode. When resizing the window in the horizontal direction, it fails to update the position of the test <div> as it should; but resize vertically as well, and it'll snap back into its proper place. I think this indicates an underlying problem whereby we fail to set IsBResize properly for the reflow-state of the top-level viewport when it has vertical writing mode (and then, of course, it doesn't propagate to descendants that may need it). So first, we should fix that, and then we can check whether the above patch is needed in addition.

First off, this: https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/layout/base/nsPresShell.cpp#9482 is surely wrong; but just changing this line to SetBSize is not sufficient to fix things, because I don't think we're getting hasUnconstrainedBSize right.

This seems to be the key place where we're currently failing to set resize flags properly, resulting in the bugs described here. (In the process of tracking this down, I've been seeing a bunch of other code that is still confused about physical vs logical resize flags, so I'll post a followup patch with some general cleanup.)

Comment on attachment 8752667 [details] MozReview Request: Bug 1272997 - Ensure block-dir resize is always respected for vertical-rl. r?jfkthame Clearing the r? flag here, as I think attachment 8752830 [details] [diff] [review] makes this unnecessary, at least for the testcases here; but let's double-check once that patch is reviewed.

I have a series of followup patches here to clean up resize-flags-related issues that I ran across during debugging this; I'll push them all to tryserver to check whether anything breaks. There's probably still more to do, but this should at least move us in the right direction. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bcb6a814a41

(In reply to Gérard Talbot from comment #1) > Better test: > http://www.gtalbot.org/BugzillaSection/Bug1272997-horizontal-resizing-VRL. > html > Clicking the Reset button of the form (after horizontally resizing the page) > unexpectedly reflows the page but does not reset form controls. Just in case this may be important. The Reset button of the form only requires a mousedown event (not even a real mouse click event) in order to unexpectedly cause the reflow the page.

By using an iframe to hold a subdocument with vertical writing-mode, and resizing it horizontally, we can use a reftest to verify the fix here; this fails with current trunk code, and passes once the fix is applied.

Thanks for taking this!

Comment on attachment 8752830 [details] [diff] [review] Set resize flags properly when calling SetComputed{Width,Height} on the reflow state for a viewport frame Review of attachment 8752830 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8752889 [details] [diff] [review] followup 1 - Check for orthogonal writing modes when propagating (logical) resize flags from parent to child Review of attachment 8752889 [details] [diff] [review]: ----------------------------------------------------------------- r=me on followup 1 -- just one nit: ::: layout/generic/nsHTMLReflowState.h @@ +602,5 @@ > bool IsBResize() const { > return mWritingMode.IsVertical() ? mFlags.mIsHResize : mFlags.mIsVResize; > } > + bool IsBResizeForWM(mozilla::WritingMode aWM) const { > + return aWM.IsVertical() ? mFlags.mIsHResize : mFlags.mIsVResize; While you're doing this, could you replace the existing "IsBResize" one-liner impl (directly above this) with a call to this new method, to avoid duplicating logic? Seems like that impl should just be replaced with: return IsBResizeForWM(mWritingMode);

(In reply to Daniel Holbert [:dholbert] from comment #21) > Comment on attachment 8752889 [details] [diff] [review] > followup 1 - Check for orthogonal writing modes when propagating (logical) > resize flags from parent to child > > Review of attachment 8752889 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me on followup 1 -- just one nit: > > ::: layout/generic/nsHTMLReflowState.h > @@ +602,5 @@ > > bool IsBResize() const { > > return mWritingMode.IsVertical() ? mFlags.mIsHResize : mFlags.mIsVResize; > > } > > + bool IsBResizeForWM(mozilla::WritingMode aWM) const { > > + return aWM.IsVertical() ? mFlags.mIsHResize : mFlags.mIsVResize; > > While you're doing this, could you replace the existing "IsBResize" > one-liner impl (directly above this) with a call to this new method, to > avoid duplicating logic? > > Seems like that impl should just be replaced with: > return IsBResizeForWM(mWritingMode); It could be, but the next patch will replace both methods anyway, as you'll see shortly. :)

Ah, great! Sorry, got pulled away for lunch + a meeting, mid-patch-stack. I'll review the remaining patches shortly. :)

Comment on attachment 8752890 [details] [diff] [review] followup 2 - Convert storage of resize flags from physical to logical Review of attachment 8752890 [details] [diff] [review]: ----------------------------------------------------------------- r=me if you split out the unrelated changes (noted below). This patch seems like it should be nsHTMLReflowState-internals-only. ::: layout/generic/nsFrame.cpp @@ +9159,2 @@ > reflowState.SetVResize(true); > + } This change (adding braces) doesn't really belong in this patch. If you want to split this into its own patch, feel free to consider that patch rs=me. Or alternately, maybe you've got another patch here that touches this chunk already; if so, feel free to fold this into that. ::: layout/generic/nsViewportFrame.cpp @@ +297,5 @@ > if (mFrames.NotEmpty()) { > // Deal with a non-incremental reflow or an incremental reflow > // targeted at our one-and-only principal child frame. > if (aReflowState.ShouldReflowAllKids() || > + aReflowState.IsBResize() || This change probably belongs in a different patch. It's not related to the core thing that's changing here (internal implementation details in nsHTMLReflowState accessors).

Comment on attachment 8752891 [details] [diff] [review] followup 3 - Use logical accessors for resize flags in nsViewportFrame Review of attachment 8752891 [details] [diff] [review]: ----------------------------------------------------------------- r=me, one nit: ::: layout/generic/nsHTMLReflowState.h @@ +842,5 @@ > + if (mWritingMode.IsVertical()) { > + ComputedWidth() = aComputedBSize; > + } else { > + ComputedHeight() = aComputedBSize; > + } I believe this if/else cascade can be collapsed to: ComputedBSize() = aComputedBSize; ...which is simpler & more directly matches the line of code that it's replacing. (At least, nsHTMLReflowState does have a ComputedBSize() accessor which returns a nscoord&. I assume that should work here.)

Comment on attachment 8752892 [details] [diff] [review] followup 4 - Set resize flags properly using logical accessors in PresShell Review of attachment 8752892 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with NS_PRECONDITION adjusted as noted below, and with another reftest if necessary (which would make the unmodified NS_PRECONDITION fail). ::: layout/base/nsPresShell.cpp @@ +1899,5 @@ > rootFrame = mFrameConstructor->GetRootFrame(); > + if (rootFrame) { > + wm = rootFrame->GetWritingMode(); > + if (wm.IsVertical()) { > + if (aWidth == NS_UNCONSTRAINEDSIZE) { So, we'll now be handling aWidth being NS_UNCONSTRAINEDSIZE here, *yet*, this function (ResizeReflowIgnoreOverride) has this at the top: > NS_PRECONDITION(aWidth != NS_UNCONSTRAINEDSIZE, > "shouldn't use unconstrained widths anymore"); So I expect whenever you hit this clause, you will have failed that earlier NS_PRECONDITION (and spammed an assertion failure). I think that NS_PRECONDITION needs a tweak, probably (to check whichever arg is our isize, instead of checking aWidth unconditionally). I also think we need a reftest that exercises that scenario (aWidth being NS_UNCONSTRAINED). I would expect that your reftest in the final patch (with an iframe) might exercise it and trip that assertion, but based on the fact that you didn't mention any assertion failures, I suspect it might not and there might be a bit more detective-work to do here...

Comment on attachment 8753041 [details] [diff] [review] Reftest for horizontal-only resizing of the document's root frame with vertical writing mode Review of attachment 8753041 [details] [diff] [review]: ----------------------------------------------------------------- r=me (though we may need an additional reftest here to exercise the previous patch's changes, as noted in comment 26). ::: layout/reftests/bugs/1272997-1.html @@ +3,5 @@ > + <head> > + <script> > + function test() { > + var elem = document.getElementById("test"); > + elem.style.width = "350px"; Nit: you've got 1-space indentation inside of this function (e.g. "var" is only indented 1 space past "function"). Indent the function-body by 1 more space, please.

(In reply to Daniel Holbert [:dholbert] from comment #26) > Comment on attachment 8752892 [details] [diff] [review] > followup 4 - Set resize flags properly using logical accessors in PresShell > > Review of attachment 8752892 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, with NS_PRECONDITION adjusted as noted below, and with another reftest > if necessary (which would make the unmodified NS_PRECONDITION fail). > > ::: layout/base/nsPresShell.cpp > @@ +1899,5 @@ > > rootFrame = mFrameConstructor->GetRootFrame(); > > + if (rootFrame) { > > + wm = rootFrame->GetWritingMode(); > > + if (wm.IsVertical()) { > > + if (aWidth == NS_UNCONSTRAINEDSIZE) { > > So, we'll now be handling aWidth being NS_UNCONSTRAINEDSIZE here, *yet*, > this function (ResizeReflowIgnoreOverride) has this at the top: > > NS_PRECONDITION(aWidth != NS_UNCONSTRAINEDSIZE, > > "shouldn't use unconstrained widths anymore"); > > So I expect whenever you hit this clause, you will have failed that earlier > NS_PRECONDITION (and spammed an assertion failure). > > I think that NS_PRECONDITION needs a tweak, probably (to check whichever arg > is our isize, instead of checking aWidth unconditionally). I also think we > need a reftest that exercises that scenario (aWidth being NS_UNCONSTRAINED). > I would expect that your reftest in the final patch (with an iframe) might > exercise it and trip that assertion, but based on the fact that you didn't > mention any assertion failures, I suspect it might not and there might be a > bit more detective-work to do here... Tweaking the NS_PRECONDITION makes sense; but I don't currently know any way to actually hit this in a reftest or similar. AFAICS, the only time we currently end up reaching this code with unconstrained aHeight is via nsDocumentViewer::GetContentSize, called from nsXULWindow::OnChromeLoaded; this happens a single time at the beginning of any reftest or crashtest run, when the first testcase is loaded. (See https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ff443f9482, where I added an unconditional assertion to flag any time we hit this code.) But with this call stack, I don't think we'll ever be dealing with vertical writing mode, as we don't support styling the whole XUL window with vertical mode. And I haven't seen any case where we hit this code with potentially-vertical HTML content such as iframes. Which suggests that some of the changes here may not be strictly necessary for any current use case, as in practice we don't hit the IsVertical() code path here. I still think we should go ahead and make the change, as it's clearly the logical thing to do, and perhaps there may be current or future scenarios that will actually hit it.

Fair enough, sounds like this would be impossible or quite hard to reftest then. I'm happy to walk back that suggestion, in that case. I agree with you that we should make the change.

Another test where (during and after) window resizing affects layout: http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/draft-multi-column-vrl-006.xht Although that test is part of another bug report, window-resizing it in various (orthogonal to the steps described here in comment 0) ways reveal the same kind of symptoms described here. If you vertical-only extent the window of that draft-multi-column-vrl-006 page, the layout of the page is updated (content is reflowed). If you vertical-only shrink the window of that draft-multi-column-vrl-006 page, the layout of the page is *not* updated; and then, here, if you horizontally resize it, then the layout is updated (content is reflowed: the bottom part of yellow content snap back into its proper place). Once this bug is fixed, we need to check window-resizing of that draft-multi-column-vrl-006 page.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4084b6e9bef3ba9d66a55466d01b92d9c9725dd0 Bug 1272997 - Set resize flags properly when calling SetComputed{Width,Height} on the reflow state for a viewport frame. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/33a81f0faa57ad12ba5433b5ffb949e90959461c Bug 1272997 - followup 1 - Check for orthogonal writing modes when propagating (logical) resize flags from parent to child. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/7160d722231cf6b814471c4e0784b6de6b40186d Bug 1272997 - followup 1a - Add missing braces, because the style guide likes them. rs=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/860520522ba2bbef1d01315b4172f7000f928932 Bug 1272997 - followup 2 - Convert storage of resize flags from physical to logical. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/3af2fb6e2b7d8d92e2740953d72fa4914bb928c9 Bug 1272997 - followup 3 - Use logical accessors for resize flags in nsViewportFrame. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfbf2d133fa6da7d99da5556b2462aa3ef6668a Bug 1272997 - followup 4 - Set resize flags properly using logical accessors in PresShell. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/a06251c9bfcb995ff03ee6389f091fe780ba833a Bug 1272997 - Reftest for horizontal-only resizing of the document's root frame with vertical writing mode. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/4084b6e9bef3 https://hg.mozilla.org/mozilla-central/rev/33a81f0faa57 https://hg.mozilla.org/mozilla-central/rev/7160d722231c https://hg.mozilla.org/mozilla-central/rev/860520522ba2 https://hg.mozilla.org/mozilla-central/rev/3af2fb6e2b7d https://hg.mozilla.org/mozilla-central/rev/5cfbf2d133fa https://hg.mozilla.org/mozilla-central/rev/a06251c9bfcb

I am using Firefox 49.0a1 buildID=20160520030251 and this bug has been fixed. marking as VERIFIED The layout issue described in comment 5 still exists (although now triggered mostly by resizing the <textarea>) and I can reproduce it even when 'writing-mode' is 'horizontal-tb'. I will create a bug report for this later. The layout issue described in comment 30 still exists. It may be related only to multi-column elements. I will create another bug report for this later.

> > 1 vertical border of text inputs is > > intermittently repainted when I bottom-right corner-resize the page. Since > > the border for text input is some kind of gradient-linear light gray > > (#eff0f1) color, this may not be quickly or easily noticeable at first. > The layout issue described in comment 5 still exists (although now triggered > mostly by resizing the <textarea>) and I can reproduce it even when > 'writing-mode' is 'horizontal-tb'. I will create a bug report for this later. That is bug 1274719: When narrowing a textarea, its border-right is repainted intermittently, creating a residual painting trace

Regression since 41, wontfix for 46 and 47 since it's pretty late in the beta cycle at this point. What do you think about aurora uplift? It may be best to keep on 49 for more time to shake out any problems. If so, let's mark this wontfix for 48 also.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #36) > Regression since 41, wontfix for 46 and 47 since it's pretty late in the > beta cycle at this point. > > What do you think about aurora uplift? It may be best to keep on 49 for more > time to shake out any problems. If so, let's mark this wontfix for 48 also. AFAICS this has existed since FF 41, and 41 was the first version where we shipped writing-mode support as enabled by default, so in terms of release-channel users it's not really a regression, but a flaw in our initial writing-mode support. While this is a nice fix to have, I don't think there's a compelling need to uplift it. The issues that arise are generally pretty minor, and fixed by just touching the window size in the other direction; and only pages that involve vertical writing mode can be affected at all, which excludes the overwhelming majority of the web. So I suggest just WONTFIX'ing for 48 as well, and let it ride the train. But if someone disagrees, note that we could fix the user-observed bug by uplifting just the first patch here, without all the internal-plumbing followups that arguably carry more risk.