Closed
Bug 1266645
Opened 9 years ago
Closed 9 years ago
Orthogonal inline block triggers warnings
Categories
(Core :: Layout: Block and Inline, defect)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla49
| Tracking | Status | |
|---|---|---|
| firefox49 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
Details
Attachments
(1 file)
Test case:
> <!DOCTYPE html>
> <div style="writing-mode: vertical-lr">
> <span style="writing-mode: horizontal-tb"></span>
> </div>
Opening the document above on Firefox would show the following in the commandline console:
> WARNING: have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation: 'NS_UNCONSTRAINEDSIZE != aReflowState.AvailableISize()', file c:/mozilla-source/central/layout/generic/nsLineLayout.cpp, line 1199
I investigated a bit, and the code triggering the warning is in nsLineLayout::AllowForStartMargin:
> } else {
> NS_WARN_IF_FALSE(NS_UNCONSTRAINEDSIZE != aReflowState.AvailableISize(),
> "have unconstrained inline-size; this should only result "
> "from very large sizes, not attempts at intrinsic "
> "inline-size calculation");
> if (NS_UNCONSTRAINEDSIZE == aReflowState.ComputedISize()) {
> // For inline-ish and text-ish things (which don't compute widths
> // in the reflow state), adjust available inline-size to account for the
> // start margin. The end margin will be accounted for when we
> // finish flowing the frame.
> WritingMode wm = aReflowState.GetWritingMode();
> aReflowState.AvailableISize() -=
> pfd->mMargin.ConvertTo(wm, lineWM).IStart(wm);
> }
> }
which is called from nsLineLayout::ReflowFrame which includes the following code just before calling that function:
> if (reflowState.ComputedISize() == NS_UNCONSTRAINEDSIZE) {
> reflowState.AvailableISize() = availableSpaceOnLine;
> }
and since ComputedISize() is not NS_UNCONSTRAINEDSIZE, AvailableISize() is not set to anything else here.
Given the code listed above, I guess the warning isn't really useful. It should probably be moved into the if-block (and we can merge the "if" with the "else"). Alternatively, the code in nsLineLayout::ReflowFrame should probably set AvailableISize() unconditionally.
|
||
Comment 2•9 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #0)
> Given the code listed above, I guess the warning isn't really useful. It
> should probably be moved into the if-block (and we can merge the "if" with
> the "else").
(Sorry for the slow response here...)
Yes, that looks reasonable.
> Alternatively, the code in nsLineLayout::ReflowFrame should
> probably set AvailableISize() unconditionally.
Possibly; I'm less sure about this, because I don't remember much detail about how these values interact. It would be interesting to know whether such a patch has any visible effect on existing testcases (either good or bad!) Maybe worth pushing a try job, and see if anything shows up?
Flags: needinfo?(jfkthame)
|
Assignee | |
Comment 3•9 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> > Alternatively, the code in nsLineLayout::ReflowFrame should
> > probably set AvailableISize() unconditionally.
>
> Possibly; I'm less sure about this, because I don't remember much detail
> about how these values interact. It would be interesting to know whether
> such a patch has any visible effect on existing testcases (either good or
> bad!) Maybe worth pushing a try job, and see if anything shows up?
When this code is initially added [1], there is a comment above:
> // Inline-ish and text-ish things don't compute their width;
> // everything else does. We need to give them an available width that
> // reflects the space left on the line.
[1] https://github.com/ehsan/mozilla-cvs-history/commit/df0d1a7e6a90fea5046968b1ba2da0045491b992#diff-e267f1823cbcb821d62973bfc015b662R730
|
|
Assignee | |
Comment 4•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53028/
Attachment #8753137 -
Flags: review?(jfkthame)
|
|
Assignee | |
Comment 5•9 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> > Alternatively, the code in nsLineLayout::ReflowFrame should
> > probably set AvailableISize() unconditionally.
>
> Possibly; I'm less sure about this, because I don't remember much detail
> about how these values interact. It would be interesting to know whether
> such a patch has any visible effect on existing testcases (either good or
> bad!) Maybe worth pushing a try job, and see if anything shows up?
FWIW, doing that doesn't seem to cause any difference in terms of reftests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c64f5a86ff7
|
|
||
Updated•9 years ago
|
Attachment #8753137 -
Flags: review?(jfkthame) → review+
|
|
||
Comment 6•9 years ago
|
||
Comment on attachment 8753137 [details]
MozReview Request: Bug 1266645 - Move around a warning to avoid triggering it for valid cases. r?jfkthame
https://reviewboard.mozilla.org/r/53028/#review49934
|
|
Assignee | |
Updated•9 years ago
|
Assignee: nobody → bugzilla
Keywords: checkin-needed
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/975596fc6440322edb04628995c7154841b22119
Bug 1266645 - Move around a warning to avoid triggering it for valid cases. r=jfkthame
|
||
Comment 8•9 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•