Closed Bug 1008019 Opened 11 years ago Closed 8 years ago

When a space overflows the width of a div, the previous word wraps as if it's attached to the div, with white-space: pre-wrap;

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Version:
29 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: guy, Assigned: jason)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 6 obsolete files)

simple-overflow-test-v2.html
1.91 KB, text/html
Details
Add reftest (currently fails) for linebreaking with white-space:pre-wrap and runs of spaces
2.74 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Update reftest and a11y-test expectations to reflect the corrected behavior of white-space:pre-wrap
8.07 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
testcase for clipPathUnits="objectBoundingBox"
732 bytes, application/xhtml+xml
Details
Allow whitespace to "hang" at soft-wrap boundaries when white-space:pre-wrap is in effect
7.88 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

Code: <div style='white-space: pre-wrap; width: 10ch; background: red; font-family: monospace;'>0000000 00 0</div>

Fiddle: http://jsfiddle.net/57ZMq/3/


Actual results:

Firefox wraps the 00 onto the next line because the space after it overflows the boundary, but the 00 itself fits onto the line. This is not how text editors typically work and breaks consistency in my web-based screenwriting app between browsers.


Expected results:

As on Chrome and Safari, the 00 should fit onto the first line and only the final 0 should wrap.
Severity: normal → major
Component: Layout → Layout: Text
Is anyone interested in this bug? This is clearly wrong behavior.
It seems like it's what is required by http://dev.w3.org/csswg/css-text/#white-space-rules unless I'm missing something, although I agree that it could be seen as undesirable.
I see what you mean, but even if it's following the semantics, I'm not sure it has to work this way (i.e. I'm not sure the other browsers are wrong, and to me their behavior is way better). I don't know rendering rules enough to say, but Chrome/Safari basically treats the space as non-breaking itself but belonging on the same line, just with no width, followed by a soft break (as permitted). So the space is tied to the previous line, but takes up no space. Maybe that technically is wrong, but realistically it's what any normal text editor does, and there's no other practical way to get it without programmatically emulating the pre-wrap behavior I expect.

Thank you for responding. I hope it can be fixed, since I've had to hack around it on Firefox, and it's causing too much trouble to really want to support.
We added rule #4 in 4.1.3 http://www.w3.org/TR/css-text-3/#white-space-phase-2 to address this. You don't want to wrap the space to the next line, but you don't want to measure the space, either.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hi all,

This bug is a serious problem for my project, so I'm going to take a crack at fixing it.

I'm building a WYSIWYG HTML5 editor. While the user types, I flip the tag/node containing the cursor between ``white-space: norrmal`` and ``white-space: pre-wrap`` as needed. I can't have this (css change) changing where words soft-wrap.


I've been programming for decades, but I'm new to the mozilla codebase.

Tips, help, mentorship, etc. are welcome.


I hope to have a patch for you within a week.

    - Jason
Attached patch collapse-ws-at-soft-wrap.patch (obsolete) — — Splinter Review 
My first crack at fixing this bug.
Above I attached my first ever patch to Mozilla. It compiles, and fixes the bug for me. I have not tested it extensively, or run any automated test suite, or anything.

I'd love feedback, review, testing, etc..
Hi! Thanks for taking a shot at digging into the codebase! One thing you should know is that the CSSWG recently (last month, actually) discussed this exact problem and concluded on a different solution.

The problem with your patch, afaict, is that it trims all of the trailing white space... which looks fine, but can be a problem if you're trying to edit said white space (particularly if there's more than one white space character). What we want is for the white space to be "visible" but not measured, kindof like hanging punctuation
  https://drafts.csswg.org/css-text/#hanging-punctuation-property

I'm not sure where is the right place to fix that, but nsLineLayout.cpp seems a likely candidate.
Alternately nsTextFrame.cpp (which you've edited here) or nsInlineFrame.cpp. I haven't really dug into it to know whereabouts, but maybe that helps. :)
Thanks for writing back! I am unreasonably happy with your response! :) Both its timelyness and content.

It took me several hours to find the right part of the codebase.

I'm so glad that I've got somebody responsive on the other side. Now there is a much higher chance of getting this work upstreamed when it's ready.

I would love to see those extra spaces be "visible" and hang. I'm glad to hear that the CSSWG is on the same page :)

Having them all be completely collapsed (as by my patch) is a bit of a problem for my project (an HTML5 editor).

Also, chromium has this correct behavior: all but the last space at the soft wrap are where you'd expect them to be: at the end of the old line... hanging off the end

I'll see what I can do :)

I suspect I need to add/pass another parameter at the same location as my previous patch and add a little more logic in the implementation of that function.
How about this patch?

I tested whitespace pre-wrap mainly. It preserves all but the last space before a soft line-break, and all the spaces at the end of the last line.

Now I can put my cursor in all the places I should be able to in my editor project.
Hanging all but the last space which then collapses... that's an option I hadn't considered. What do you think of that vs just hanging all of the spaces so that none of them are invisible? Also, what about trailing tab characters?
huh? The last one becomes the newline. This is how everything works.

Try it in this comment box, or in word, or anywhere.

When you have just one space between words, and it wraps, you can put your cursor immeditaly next to (after) the last word on the line, then you press right-arrow, and your cursor is at the beginning of the next line.

When you add one more space (so there are two at the soft wrap) you shouldn't suddenly be able to put your cursor after two spaces at the end of the first line, when you there was no "space" there a moment earlier when the words were separated by a single space char.
My apologies if my last comment was confusing or unpleasant, I wrote it in a rush.

I'm still excited about this :) and happy that you're engaging in this conversation :)

Below I put forth my thoughts and opinions about what currently makes sense to me to do with spaces at a soft wrap. I would love to expand my thinking by hearing your thoughts, and those of the CSSWG.

I have no idea what to do with a tab before a soft-wrap, that doesn't make any sense to me. I just tested what LibreOffice does, and it's terrible (sometimes the cursor jumps to a different location when you type). I am not planning to make it easy for people to insert tab characters in my editor.

Perhaps my thinking below only applies if the last white-space character is a normal space or some kind of newline character.


I do think it is important that the last space before a soft wrap be "displayed" only as if it were a newline; because:

1. I think there is a lot of consistency in the world across different text-editing programs-widgets

2. It makes sense to me, and I think it's a good design, that the number of places you can put your cursor corresponds to the number of insert positions (places between/before/after characters). When there is a soft wrap between two lines at a single space, you can place the cursor at the end of the first line (before the space) or at the beginning of the second line (after the space)

3. When the user adds a second space, I think it is important for consistency to only add one additional space to the display.

I've been a little reluctant to post links to my project, because it is changing so fast, and sometimes it's useful for links in bug reports to stay static for a very long time, but... I'm going to anyway, because it lets you interact with spaces in white-space: normal and white-space: pre-wrap in meaningful ways.

Try this: https://jasonwoof.com/peach-editor/editor_tests_compiled.html

click within the text of the first paragraph in the WYSIWYG area and type away, play particularly with spaces at the end before a soft-wrap.

As you edit, you can see the html source code changing in the textarea above. It switches between white-space: normal and pre-wrap as needed to get the spaces that you type to render as I (and hopefully most people) would expect.


That is the project that is my motivation for working on this bug. I weighed the pros and cons of a few different ways of getting space not to collapse as the user types, particularly space that is immediately next to the cursor. I think using pre-wrap is the best option (by a significant margin), assuming we can get this bug resolved in a similar way to chrome/etc.

Thank you! :)

   - Jason
I wanted to clarify my notes on the editor demo link above.... That demo behaves how I think it should (and how I think most text editors behave) in chrome, but it only behaves how I think it should (and how I described it) in firefox after applying my latest patch.
What can I do to get this merged?

What's the next step?

Dave: what info do you need?


This patch is obviously a huge improvement over current behavior, and fixes the test case provided on this bug report.

Are we concerned about dangling tabs? Or not dangling the last character (treated it is a linebreak character)? I did the common-sense thing, which is also the behavior in chrome. I believe this is what one should do in the absence of a clear spec.


If there needs to be extensive debate (or prolonged indecisive silence) about what do do with the last character, or how to update Firefox according to an update from CSSWG last month, I urge you all to do that after this patch is merged, possibly in a separate bug report. Such fiddly little edge cases shouldn't stop us from fixing the majority of cases (possibly all of them), and getting to parity with the most popular browser.
Jason, the patch needs review from a module owner or peer.  See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Module_ownership and the "Getting reviews" bit on the same page.

David is the module owner for the "layout engine" module; see <https://wiki.mozilla.org/Modules/Core#Layout_Engine>.  The needinfo on himself is to remind him to look at this patch.  Unfortunately, he's out of town right now and "less responsive until April 4", as his name above says; I expect he'll be able to look at this in more detail when he gets back.

In the meantime, I've gone ahead and pushed your patch to the try server so we can at least see the results of an intergration test run on it.  Results should start appearing at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ff56d0e6842 in a bit.
OK, some failures from that run:

1)  Running editor/libeditor/tests/test_bug596333.html (mach mochitest editor/libeditor/tests/test_bug596333.html) triggers an unexpected assertion: ###!!! ASSERTION: If the text doesn't fit, and we have a break opportunity, why didn't MeasureText use it?: 'textMetrics.mAdvanceWidth - trimmableWidth <= aAvailableWidth', file /builds/slave/try-lx-d-000000000000000000000/build/src/layout/generic/nsTextFrame.cpp, line 8827

This assertion also pops up in some other test suites, but they're probably more complicated to debug than this test.

2) Various failures in <chrome://mochitests/content/a11y/accessible/tests/mochitest/text/test_atcaretoffset.html>.  The first one is:

  getTextAfterOffset (line start): wrong text (got 'hello ', expected: 'hello'), offset: -2, id: 'ta_wrapped' ;

and the rest might well be just cascading effects of offsets being off from expected.

3) The layout/reftests/text/justification-1.html refrest fails.  Looking at http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/firefox/try-builds/bzbarsky@mozilla.com-1ff56d0e6842ac75168a42a37f12ee15ecda0b31/try-linux-debug/try_ubuntu32_vm-debug_test-reftest-4-bm08-tests1-linux32-build636.txt.gz&only_show_unexpected=1 the failure is that in the fourth paragraph the word "model" ends up wrapping when it actually fits on the previous line.

4)  The layout/reftests/text/wordwrap-01.html reftest fails.  Looking at the same reftest analyzer url, the space before "Firefox" is now on the previous line, not at the beginning of the next one; this may be a purposeful change from this patch, of course.
That's so good! Thanks for explaining :)

Thanks also for pushing to try, I remember reading that that's a useful step, and one that I don't have access to do myself.

Thanks also for details, links and comments on the try results, I'm still figuring out how to use try, and generally find things. Below are my responses to your points:

1) "ASSERTION: If the text doesn't fit, and we have a break opportunity, why didn't MeasureText use it?"

The new correct behavior is to have the text overflow ("hang" in CSSWG terminology) when there are sufficient extra spaces at a word wrap.

This violates the old expectation that text should not simultaneously both overflow and contain a break opportunity.

There may be an opportunity here for me to fudge the metrics in a different way than the original code.


2) The new behavior is correct. Spaces that don't fit at the end of a line should overflow, not be moved to the beginning of the next line.

3) The new behavior is correct. Clearly "model" fits on the higher line.

4) Yes, this is on purpose. The new behavior is what I expect (matches my understanding of how this should work) and matches the behavior in Chromium.


So it seems like there are a fair amount of assertions, tests, etc that need to be updated. Questions about that:

1. Should I wait far David to take a look at my patch before I work on updating the assertions/tests/etc?

2. Should the updates to the assertions/tests/etc be in the same patch as the behavior change, or a separate patch?

Thanks!
> This violates the old expectation that text should not simultaneously both overflow 
> and contain a break opportunity.

OK.  The assertion may need to be adjusted to handle this case...

> The new behavior is correct.

The test probably needs to be updated, then.  And it's good to have an a11y developer look over the resulting test changes to see whether the results make sense or whether adjustments to the a11y code are needed as a result.

> 3) The new behavior is correct. Clearly "model" fits on the higher line.

Ah, good point.  I had missed that it was on the next line in the _reference_, not the test.

It probably makes sense to wait for David before updating tests, though updating the assertion might be a good idea because he might be interested in what the updated assertion looks like.  As far as patch mechanics once you do update things, it would need to land as a single changeset so that bisecting doesn't have bad states with failing tests.  But past that, whatever you think would make reviewing or your life simpler; multiple patches attached to this bug can be folded into a single changeset as needed.
Thanks!

I'm not sure if it'd be best to delete the assertion, or return an additional metric from BreakAndBeasureText that would enable the assertion to be fixed.

I'm happy to do more work on this, but I think I'll take your suggestion and wait for David to take a look.
(In reply to Jason Woofenden from comment #14)
>
> I'm still excited about this :) and happy that you're engaging in this
> conversation :)

Sorry! Totally forgot about this conversation. (I'm not particularly active in the Mozilla project anymore, so I tend to ignore bugmail.)

> Below I put forth my thoughts and opinions about what currently makes sense
> to me to do with spaces at a soft wrap. I would love to expand my thinking
> by hearing your thoughts, and those of the CSSWG.

This is useful, would you mind if I forwarded to www-style?

> I have no idea what to do with a tab before a soft-wrap, that doesn't make
> any sense to me. I just tested what LibreOffice does, and it's terrible
> (sometimes the cursor jumps to a different location when you type). I am not
> planning to make it easy for people to insert tab characters in my editor.

Fair enough, but here on the Web platform, we have to deal with all of the possible crazy inputs. =)

I would imagine the most common reason for a tab character in the middle of the line would be TSV files. In this case, it probably makes sense to just wrap the tab to the next line, so that's probably a reasonable thing to do here.

> I do think it is important that the last space before a soft wrap be
> "displayed" only as if it were a newline; because:
> 
> 1. I think there is a lot of consistency in the world across different
> text-editing programs-widgets
> 
> 2. It makes sense to me, and I think it's a good design, that the number of
> places you can put your cursor corresponds to the number of insert positions
> (places between/before/after characters). When there is a soft wrap between
> two lines at a single space, you can place the cursor at the end of the
> first line (before the space) or at the beginning of the second line (after
> the space)
> 
> 3. When the user adds a second space, I think it is important for
> consistency to only add one additional space to the display.

Agreed on the third point for sure. The one concern I have is with display modes that "show" the space, e.g. as a faint middle-dot. I imagine this could be done with a font that has a visible space glyph... wrapping and making the space disappear would cause that representation to also disappear. But I think #2 is also a reasonable argument in the other direction, and of course so is interop.

I'll take a closer look over your patch later today or tomorrow. But one thing you do need to fix is to include some reftests! The tests should go in here: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/text3/ Basically you're making two files that should render identically, one without this feature and one that's testing it. Make sure you test what happens with: spaces, non-breaking spaces, tabs, line feeds, form feeds, zwsp, mixtures thereof, and the rest of the stuff in Zs http://www.fileformat.info/info/unicode/category/Zs/list.htm (most of which, I think, shouldn't hang, but we need to check that they don't hang).
Okay, so, here's my main review comment on the patch: why are we adding aHangWhitespace and the subsequent logic instead of reusing aTrimWhitespace? White space seems to behave as we want when it's not preserved: it's measured and selectable, but doesn't affect wrapping. It seems to me replacing

   bool canTrimTrailingWhitespace = !textStyle->WhiteSpaceIsSignificant() ||
                                    (GetStateBits() & TEXT_IS_IN_TOKEN_MATHML);

with

   bool canTrimTrailingWhitespace = !textStyle->WhiteSpaceCanWrapStyle() ||
                                    (GetStateBits() & TEXT_IS_IN_TOKEN_MATHML);

would actually fix this bug.
Thanks for looking at the code and my patch!

I tried something similar to that initially, but the whitespace was not selectable (it appeared to be collapsed, not hanged).

There is a significant possibility that I missed something, but this solution did not work, at least not in the way I need it to for my project. My project uses the range api to find the location of text characters, and when I did a patch similar to yours above, the browser was returning exactly the sort of bogus ClientRects that it does for whitespace characters that are not displayed at all because they are collapsed.

My patch makes the text wrapper thing report a different width, so the hanging spaces fit within the reported width. I don't understand why this makes the characters selectable, but it does.
OK, so I'm trying to understand the state of things here.  My understanding is that:

 (1) I should ignore attachment 8732652 [details] [diff] [review] and consider attachment 8733592 [details] [diff] [review].  (If that's correct, could you edit-details on the obsolete one and mark it as obsolete?)

 (2) The spec text that's referenced in comment 4 is actually obsolete (because the link is to TR), and has been replaced by the resolution mentioned in the first paragraph in comment 8, which I believe is https://lists.w3.org/Archives/Public/www-style/2016Mar/0352.html , and I believe this resolution is now up-to-date in the editor's draft at https://drafts.csswg.org/css-text/#white-space-phase-2 (but the TR-draft -- a permalink to the current draft there is https://www.w3.org/TR/2013/WD-css-text-3-20131010/#white-space-phase-2 -- is still out-of-date).

 (3) The intent of this patch is that for 'white-space' values that allow white space to collapse, we should still trim whitespace at the end of the line.  But the new behavior is that for 'pre-wrap', we should instead allow whitespace at the end of the line to hang past the edge of the line box, rather than causing the word preceding it to wrap.  This behavior for pre-wrap is what the patch intends to change.


I'm a bit confused by the comments in the gfxTextRun.cpp part of the patch.  It seems to imply that when aHangWhitespace is true, then one character of whitespace is trimmed (and becomes a newline).  I don't understand this.

I also find the terminology in the patch a bit confusing; I think the spec terminology ("white space hangs", where "white space" is the subject of the verb "hang") makes more sense than the patch's terminology ("hang white space", where "white space" is the direct object of the verb "hang").  This could be fixed by some simple name changes, e.g., changing aHangWhitespace to aWhitespaceCanHang.

I suppose the major design decision to be made in the patch is which measurements should actually change as a result of this behavior of allowing whitespace to hang (i.e., which sizes shouldn't include the whitespace that can hang, and which sizes should include that whitespace).  My intuition is that things are more likely to work out sensibly if we end up making the sizes of text frames include the whitespace that they contain, but just place them on the correct line (with the whitespace overflowing).  I *think* (although I'm not sure) that this patch takes the opposite approach, and actually shrinks the text measurement and thus the size of the text frame.  I worry this might cause problems with things like caret positions -- but if it doesn't, maybe that's fine.

I guess the alternative design I would imagine would involve, instead, having the gfxTextRun code *report* the size of what could be allowed to hang at the end of the frame, rather than subtracting it from the measurement, and then having the nsTextFrame code deal with allowing the text to be positioned at the end of the line despite its size.  I suspect such a design might extend better to things like hanging punctuation.  But it might also be more work and not worth doing now.  If things like caret positioning and editing work correctly with your approach, and you're able to get the tests to pass, then it's probably fine to continue with this approach, since it's quite simple.  But it might also be worth thinking about the alternative.

I also don't feel like I'm particularly the expert on this code; I realize that I should perhaps have deferred (weeks ago!) to Jonathan (who I've now cc:ed).  He may or may not have a different opinion.

In any case, thanks for working on this, and sorry for taking so long to get back to you.
Flags: needinfo?(dbaron)
Attachment #8732652 - Attachment is obsolete: true
Hi David,

Thank you for or review, attention, and clarity. I hope your "less
responsive" time was spent somewhere enjoyable :)

I'm excited to see this move forward.


>  (1) I should ignore attachment 8732652 [details] [diff] [review] and consider attachment 8733592 [details] [diff] [review].  (If
> that's correct, could you edit-details on the obsolete one and mark it as
> obsolete?)

Correct. I marked it as obsolete (wow that checkbox was hard to
find.)


>  (2) The spec text that's referenced in comment 4 is actually obsolete (because
> the link is to TR), and has been replaced by the resolution mentioned in the
> first paragraph in comment 8, which I believe is
> https://lists.w3.org/Archives/Public/www-style/2016Mar/0352.html , and I
> believe this resolution is now up-to-date in the editor's draft at
> https://drafts.csswg.org/css-text/#white-space-phase-2 (but the TR-draft -- a
> permalink to the current draft there is
> https://www.w3.org/TR/2013/WD-css-text-3-20131010/#white-space-phase-2 -- is
> still out-of-date).

Thank you for collecting these links and saying which are current.
I know next to nothing about the process of these specs evolving.

I prefer the language here:
https://lists.w3.org/Archives/Public/www-style/2016Mar/0352.html

Specifically this:

	In order to prevent overflow or wrapping of invisible white
	space, spaces at the end of a line must either be visually
	collapsed to fit within the line or must hang outside the line
	(as in hanging punctuation)

I like the phrase "outside the line" because when the spaces at the
end are within the line/container, it seems pretty obvious that
they should be preserved.

But when they don't fit, I understand how people would want them to
either collapse or hang in different circumstances. eg if you're
editing, you want them to hang. If not editing/selecting and
hanging would create a horizontal scrollbar to appear, you would
probably want it to collapse, or at least have the characters that
don't fit collapse.



>  (3) The intent of this patch is that for 'white-space' values that allow white
> space to collapse, we should still trim whitespace at the end of the line.  But
> the new behavior is that for 'pre-wrap', we should instead allow whitespace at
> the end of the line to hang past the edge of the line box, rather than causing
> the word preceding it to wrap.  This behavior for pre-wrap is what the patch
> intends to change.

Correct.

Note that sometimes whitespace at the end of a line does fit within
the line box, and the patch also preserves such spaces/tabs/etc.


> I'm a bit confused by the comments in the gfxTextRun.cpp part of the patch.  It
> seems to imply that when aHangWhitespace is true, then one character of
> whitespace is trimmed (and becomes a newline).  I don't understand this.

fantasai didn't get this either. So I've been thinking about it
more... I think I did the wrong thing here.

I will make an updated patch that preserves the final whitespace
character too.

If you're interested, here is how I came to that line of thinking:

	In most text editing apps (outside the html/css world) when an
	automatic linebrake is displayed, it is displayed instead of a
	space. (note: I'm currently no longer sure this is true)

	Note that such apps do not collapse spaces.

	Thus the process of wrapping text to multiple lines is
	replacing a space with a newline.

	When there are multiple spaces in a row at the break point, the
	extra spaces are left on the previous line... usually? also the
	new spec stuff says to hang them, not put them at the start of
	the next line

	So, refined: the process of wrapping text to multiple lines is
	replacing the last space at a break opportunity with a newline

That was the old thinking.

Here's what I've come to, which I think matches what other folks
here are thinking, and what the CSSWG folks are saying:

	HTML/CSS world, with collapsible space:

		a newline is inserted/rendered between 1+ spaces and a
		non-space character

		whitespace characters preceding the break are collapsed
	
	There is no "replacing with a newline" in this process, thus
	the process for wrapping with non-collapsible space should be:

		a newline is inserted/rendered between 1+ spaces and a
		non-space character

		This break position is chosen such that as much non-space
		characters as possible fit within the box, even if the
		space character(s) don't fit also.

		Space characters at the end that fit should definitely be
		rendered (so you can see when you're selecting them, have
		sane cursor positions for editors, etc)

		Space characters that do not fit should probably hang off
		the edge, though this is more a matter of opinion, as there
		are cases where it is undesirable.


> I also find the terminology in the patch a bit confusing; I think the spec
> terminology ("white space hangs", where "white space" is the subject of the
> verb "hang") makes more sense than the patch's terminology ("hang white space",
> where "white space" is the direct object of the verb "hang").  This could be
> fixed by some simple name changes, e.g., changing aHangWhitespace to
> aWhitespaceCanHang.

I like it. :)


> I suppose the major design decision to be made in the patch is which
> measurements should actually change as a result of this behavior of allowing
> whitespace to hang (i.e., which sizes shouldn't include the whitespace that can
> hang, and which sizes should include that whitespace).  My intuition is that
> things are more likely to work out sensibly if we end up making the sizes of
> text frames include the whitespace that they contain, but just place them on
> the correct line (with the whitespace overflowing).  I *think* (although I'm
> not sure) that this patch takes the opposite approach, and actually shrinks the
> text measurement and thus the size of the text frame.  I worry this might cause
> problems with things like caret positions -- but if it doesn't, maybe that's
> fine.

I don't understand much about the different types of measurements,
but what I thought I accomplished with this patch was returning
larger measurements, such that they included the spaces at the end
of lines (-1 unfortunately, I'll fix that.)

When the spaces are collapsible, the code (both pre and post my
patch) returns a measurement that does not include the spaces at
the end of the line.

The primary change of my patch is to return a larger measurement
for the text run when space is to be preserved.

There do have to be two measurements, one to see if the non-space
characters fit, and one to say how big the text line is, including
the spaces at the end.

It's possible that the first measurement is somehow reaching the
calling function (and getting used for something), I'm not sure. I
was hoping somebody with more understanding of the bigger picture
would take a look, so my guess here wouldn't fowl something up.


> I guess the alternative design I would imagine would involve, instead, having
> the gfxTextRun code *report* the size of what could be allowed to hang at the
> end of the frame, rather than subtracting it from the measurement, and then
> having the nsTextFrame code deal with allowing the text to be positioned at the
> end of the line despite its size.  I suspect such a design might extend better
> to things like hanging punctuation.  But it might also be more work and not
> worth doing now.  If things like caret positioning and editing work correctly
> with your approach, and you're able to get the tests to pass, then it's
> probably fine to continue with this approach, since it's quite simple.  But it
> might also be worth thinking about the alternative.

I like your thinking here.

I think my code implements the alternative you describe.

And it does work well for caret positioning in my app (I'm not
using the browser carrot, just the range/selection API to find out
where the characters are, and absolute positioning a cursor.)


> I also don't feel like I'm particularly the expert on this code; I realize that
> I should perhaps have deferred (weeks ago!) to Jonathan (who I've now cc:ed). 
> He may or may not have a different opinion.

Oh, thanks for bringing him on.


> In any case, thanks for working on this, and sorry for taking so long to get
> back to you.

No problem, I got patient after a while :)

-- 
Jason
Correction: browsers (I just tested chrome and firefox) don't collapse the final space before a linebreak. But browsers hang that last space (I can select it with the mouse... though in chrome I can only select it by moving the mouse cursor to the next line... ie I have to move the mouse as if selecting a line break, but the selection visibly looks like it's hilighting a space.)
(In reply to Jason Woofenden from comment #26)
> 	HTML/CSS world, with collapsible space:
> 
> 		a newline is inserted/rendered between 1+ spaces and a
> 		non-space character

Though there can also be breaks where there aren't space characters, e.g., after a hyphen, or in the middle of a run of Chinese characters.

> 		whitespace characters preceding the break are collapsed

In this case I think we tend to use the terminology "trimmed" rather than "collapsed", since collapsed refers to collapsing multiple whitespace characters to a single whitespace character, whereas trimming refers to removing all the whitespace (leaving 0 whitespace characters), at least from the measurement that we care about for breaking.

> It's possible that the first measurement is somehow reaching the
> calling function (and getting used for something), I'm not sure. I
> was hoping somebody with more understanding of the bigger picture
> would take a look, so my guess here wouldn't fowl something up.

OK, I guess I just hadn't looked closely enough at your patch to BreakAndMeasureText to figure out whether it was influencing the only the breaking part, or both the breaking and measuring part.  If so, that sounds great.

(In reply to Jason Woofenden from comment #27)
> Correction: browsers (I just tested chrome and firefox) don't collapse the
> final space before a linebreak. But browsers hang that last space (I can
> select it with the mouse... though in chrome I can only select it by moving
> the mouse cursor to the next line... ie I have to move the mouse as if
> selecting a line break, but the selection visibly looks like it's hilighting
> a space.)

Yes, whitespace trimming does funny things when combined with editing and selection.


I guess the one other question, though, is how what we want to do for pre-wrap is different from the trimming that we currently do for the cases where whitespace can collapse.  Your comments make it sound less different than before, which makes me wonder whether the code should be more similar.
Thanks for details, examples and terminology. I'm new to this
stuff, and especially new to talking about it.


> Your comments make it sound less different than before, which
> makes me wonder whether the code should be more similar.

I'm not sure what you mean here (make what more similar to what?)

But I'm hoping you mean that the code that does the breaking should
be the same (or as close to it as reasonable) whether we're
collapsing spaces or not. I believe my patch achieves this.

I hope you'll take a closer look at my work after I get a chance to
fix that peculiarity that has it trimming one space at most breaks.

I wish I could have done that this week, but I've been unusually
busy. I'm glad I found the time to continue this conversation
though, because now I know what to do :)
Attachment #8733592 - Attachment is obsolete: true
Sorry for the delay. I was sick for a while.

Attached is an updated patch which preserves the final space before breaks, as discussed above.

Also I renamed variables as suggested by dbaron.

I am quite happy with how this renders text with pre-wrap in paragraphs and such. I get good cursor positions in my editor, etc.

Now that single spaces before breaks (a very common case) hang, it becomes clear that this patch does terrible things to textareas (specifically, adding a horizontal scrollbar a lot of the time.

I suspect this is a problem generally with anything that has overflow: auto.

I am not sure how this new issue should be addressed.

Please advise.

I like the behaviour that chromium has for textareas when there are too many spaces to fit at a break. Chromium displays as many spaces as fit as normal, and then acts as though the rest of the spaces (the ones that don't fit, and must collapse (visually at least) or hang) as if they had zero width. This makes it so the cursor never goes off the edge, and you don't get a horizontal scrollbar.

That seems like the best possible behavior.

In current firefox, the behavior in this situation is to move the break one word earlier, so the large string of spaces and the word preceding it are moved to the next line. This is obviously wrong.

After my patch, the firefox behavior is to add a horizontal scrollbar to the textarea, which is unpleasant.


My hunch is that hanging whitespace should not affect the size of the containing element (for example it should not trigger scrollbars, or the parent's scrollWidth). I have no idea how to achieve this.

I just poked around in chromium a bit, with p tags with white-space: pre-wrap. the whitespace does hang past the border of the element, but this extra space does not change the element's scrollWidth property. Note that if there is a single word that is wider than the element's width, scrollWidth does increase.
Comment on attachment 8747193 [details] [diff] [review]
display whitespace in pre-wrap/etc at breaks

posting replacement patch shortly
Attachment #8747193 - Attachment is obsolete: true
Finally a good solution hit me!

The attached patch implements a vast improvement in the rendering of spaces at the end of a line in elements that allow wrapping but preserve space. I've tested with normal ``textarea``s and ``p`` and ``h1`` tags with ``white-space:pre-wrap``.

I found a way to make this improvement without causing scrollbars or other overflow-related problems: allow whitespace at the end of lines (before any kind of break, or the end of the containing block) to take up as much space as is available (without overflowing the container) and do _not_ allow them to take up space past the edge of the container.

This is achieved by fiddling with the metrics returned by the function that breaks/measures text.

Note: the old implementation of trimming characters fiddles with the metrics in the same way.

I am not aware of any bugs/problems with this patch, so unless further issues are discovered, I think it is ready to be merged.

Here's the testing I've done:

Testing with a default (no css) ``textarea``:

    put lots of text in, resize textarea with mouse. Result: no horizontal scrollbar. Good, this means that this version of my patch does not introduce this regression like the previous version did.

    put lots of spaces before word-wrap breaks, and also before hard (\n) breaks. When I do this, I get the intended behavior: as I press space, the cursor moves right (each time) until it reaches the right edge of the box, and then it doesn't move on further pressing of space. It behaves as though the spaces that don't fit have a width of zero (when a space would partially fit, it is treated as though it were the width of the remaining space). I can select with the mouse, and I can select all the spaces (copy/paste works as expected).

Testing with ``p`` and ``h1`` in my editor project: In this project I'm using the browser's range api (``r = document.createRange()``, ``r.setStart()``, ``r.setEnd()``, ``r.getClientRects()``) to find the locations of individual characters in order to place the (fake) cursor in the correct location.

    play with putting lots of spaces before word wraps and at end. The cursor is behaving exactly as described above with the real cursor of text areas. This means that the browser is reporting good locations when asked where those spaces are.

    Test that toggling the ``white-space: pre-wrap`` property (adding and removing it) does not change where automatic word-wrapping breaks. This is problematic without my patch. Fixing this is the main motivation for me writing these patches.


I look forward to your reply.
Thanks again for your feedback and suggestions.

I think this latest patch (finally) does something very reasonable that we can all agree on.

What's the next step?

I'm willing to spend some time on creating/updating tests, but I'd first like some assurance that we seem to be on track for getting this merged.

Of course, if somebody who knows what they're doing can work on the tests, I'd much prefer that.

I'm on a 32-bit system, so there's a possibility I won't be able to build the test stuff.

Thanks all.
Flags: needinfo?(dbaron)
Sorry for letting this fall off my radar again.

In the future, using needinfo? flags is a better way to get people's attention.  I've also given you some additional Bugzilla permissions.

(In reply to Jason Woofenden from comment #32)
> I found a way to make this improvement without causing scrollbars or other
> overflow-related problems: allow whitespace at the end of lines (before any
> kind of break, or the end of the containing block) to take up as much space
> as is available (without overflowing the container) and do _not_ allow them
> to take up space past the edge of the container.

So the spec says:
  # If spaces or tabs at the end of a line are non-collapsible but have
  # white-space set to pre-wrap the UA must either hang the white space or
  # visually collapse their character advance widths such that they don’t
  # take up space in the line.
If I'm understanding correctly, it seems like what you're doing here is neither the "either" or the "or" in that prose, but a combination of the two.  I think that's probably reasonable, although the spec doesn't currently allow it.  We should probably ask for the spec to be changed.

Do you know how this compares to the behavior in other browsers?

>     Test that toggling the ``white-space: pre-wrap`` property (adding and
> removing it) does not change where automatic word-wrapping breaks. This is
> problematic without my patch. Fixing this is the main motivation for me
> writing these patches.

Presumably it does change the wrapping if there are multiple spaces in a row, not at the end of a line, right?



As far as intent to merge the patch goes:  I think we do want to fix it, given that the spec says our current behavior is wrong, at least assuming that doing so wouldn't cause compatibility problems.  Generally a good sign that it won't cause compatibility problems is that the behavior moves us closer to the behavior of other browsers.  It sounds like that's the case, although I'd like to confirm.

I suspect Jonathan will be a better reviewer for the patch than I, though.
Yay :) great to see movement on this

OK, I'll try the needinfo? flag if/when I need to nudge again.

> (In reply to Jason Woofenden from comment #32)
> > I found a way to make this improvement without causing scrollbars or other
> > overflow-related problems: allow whitespace at the end of lines (before any
> > kind of break, or the end of the containing block) to take up as much space
> > as is available (without overflowing the container) and do _not_ allow them
> > to take up space past the edge of the container.
> 
> So the spec says:
>   # If spaces or tabs at the end of a line are non-collapsible but have
>   # white-space set to pre-wrap the UA must either hang the white space or
>   # visually collapse their character advance widths such that they don’t
>   # take up space in the line.
> If I'm understanding correctly, it seems like what you're doing here is
> neither the "either" or the "or" in that prose, but a combination of the
> two.  I think that's probably reasonable, although the spec doesn't
> currently allow it.  We should probably ask for the spec to be changed.
> 
> Do you know how this compares to the behavior in other browsers?

Chromium seems to behave much like my patch, ie it collapses only the part of
the whitespace that extens beyond the containing box. I haven't done thorough
testing on this. I'll shortly attach a simple test page.


> >     Test that toggling the ``white-space: pre-wrap`` property (adding and
> > removing it) does not change where automatic word-wrapping breaks. This is
> > problematic without my patch. Fixing this is the main motivation for me
> > writing these patches.
> 
> Presumably it does change the wrapping if there are multiple spaces in a
> row, not at the end of a line, right?

Correct.


> As far as intent to merge the patch goes:  I think we do want to fix it,
> given that the spec says our current behavior is wrong, at least assuming
> that doing so wouldn't cause compatibility problems.  Generally a good sign
> that it won't cause compatibility problems is that the behavior moves us
> closer to the behavior of other browsers.  It sounds like that's the case,
> although I'd like to confirm.

Great! I belive this patch gets us much closer to other browsers, and closer to
the spec (deviating from the spec only in ways that chromium does).


> I suspect Jonathan will be a better reviewer for the patch than I, though.

Great. I don't know his email so I can't needinfo? him.


I'm up for making more thorough test pages for comparing the behavior of
different browsers. I'm just not sure at this point if that's helpful. Let me
know if I should do this.

-- 
Jason
Attached file simple-overflow-test.html (obsolete) — 
this is a very simple test page you can interact with in browser to see if/how selections (visually) extend within/beyond pre-wrap elements before word-wrap breaks, and also how cursors behave with spaces up to the right edge of a textarea.
When testing simple-overflow-test.html in chromium, I get some quirks:

1. Sometimes selection (blue background) is shown outside the div box, sometimes not. When I clear the selection by clicking elsewhere on the page, if it's showing blue outside the box, this blue outside the box remains after the rest of the selection disappears. This is clearly a bug.

2. Chromium has a very odd quirk: in the textarea, when I follow the instructions so I've pressed space near the end of the line enough to overflow... at this point everything is normal (the cursor is at the right edge, no scrollbars) but then if I press enter, chromium creates scrollbars so I can scroll right and see all the spaces that I made (that I couldn't see while typing them). My patch does not cause scrollbars.

These tests were done on Chromium version 51.0.2704.79
(In reply to Jason Woofenden from comment #36)
> > As far as intent to merge the patch goes:  I think we do want to fix it,
> > given that the spec says our current behavior is wrong, at least assuming
> > that doing so wouldn't cause compatibility problems.  Generally a good sign
> > that it won't cause compatibility problems is that the behavior moves us
> > closer to the behavior of other browsers.  It sounds like that's the case,
> > although I'd like to confirm.
> 
> Great! I belive this patch gets us much closer to other browsers, and closer to
> the spec (deviating from the spec only in ways that chromium does).

Sounds good.

> > I suspect Jonathan will be a better reviewer for the patch than I, though.
> 
> Great. I don't know his email so I can't needinfo? him.

":jfkthame" will autocomplete.

> I'm up for making more thorough test pages for comparing the behavior of
> different browsers. I'm just not sure at this point if that's helpful. Let me
> know if I should do this.

Depends if you think there are other cases that seem worth testing.  I don't think there's a need for lots of tests just for the sake of testing lots of combinations, but if you think there are combinations that are interesting or have a decent chance of being different in some way, then they're more likely worth testing.


Also, I raised https://github.com/w3c/csswg-drafts/issues/277 as an issue on the spec.
Flags: needinfo?(jfkthame)
Looking at the testcase (attachment 8768532 [details]), I'm finding myself unsure what the selection-highlighting result here is intended to be. In both Safari and Chrome, selecting all the text in the top box results in highlighting that extends far beyond the right-hand edge of the box, so that it covers all the spaces that are "hanging" at the line break, even though they project beyond the box. This seems to contradict the description above that box.

In current Firefox, we wrap the text so that no spaces overflow/hang, which is the undesired behavior you're fixing here; so I tried it with the current patch. That does indeed give wrapping that matches Safari and Chrome, but again, if I select the text, I see the "hanging" spaces all highlighted even though they extend beyond the box. So this is similar to the webkit/blink browsers, but again, it does not correspond to the description given in the testcase. Was that mistaken, or has there been a change to the intended behavior, or....?
Flags: needinfo?(jfkthame) → needinfo?(jason)
Attachment #8768532 - Attachment is obsolete: true
new test page to reflect updated expectations after hearing back from css spec folks and updating patch
Hi Jonathan,

Thanks for looking at this!

It sounds like everything is correct except the description in my testcase (attachment 8768532 [details]).

I (just now, see above) uploaded a fixed version of that test case. I changed the descriptions/instructions and varied the amount of whitespace in the box a bit.

We heard back from the css folks at https://github.com/w3c/csswg-drafts/issues/277 who said that "hanging" (ie extending beyond the bounding box) is the most correct behaviour for whitespace at line end.

I am not overly confident that I've implemented this in the best way. My primary motivation is to get firefox to break in the right place with pre-wrap, but got sucked in to also getting the spaces to do the write thing. As you can see my patch makes only minor changes (changing only how the break point is calculated, and what measurement is delivered to the caller).

If this approach to creating hanging whitespace turns out to not be OK for some reason, and a lot more work needs to be done to make that happen in a better way, then I would plead to have at least the part of this patch that fixes the breakpoint be merged. Having pre-wrap text break at the wrong places is breaking my app, and is the problem that this bug report was originally about.
Flags: needinfo?(jason) → needinfo?(jfkthame)
Thanks for the update and clarification; I saw the CSS-WG comment, too. From a user point of view, the "hanging beyond the box" behavior seems a bit odd (IMO), but is probably OK - at least, it's not clear that any alternative would be better. Though it does seem strange that the <div> allows highlighting to extend beyond the box, while the <textarea> doesn't; it clips the highlighting of hanging spaces to the element's box.

More disturbing, perhaps, is that if the <div> is given the contentEditable attribute, it's now possible to move the blinking caret out into the hanging spaces, beyond the element's rect, which looks really weird. And Firefox (with patch) responds to mouse events there, so that I can drag-select across the hanging spaces even though they're outside the element's box; yet the resulting selection doesn't respond to keystrokes as one would expect, because the event occurred outside contentEditable element and deactivated its editor.

Anyway.... I need to spend a bit of time actually looking at the patch (as well as re-reading the discussions), not just experimenting with the behavior. I'll try to get back to you soon. Leaving the needinfo flag for now, as a reminder.
yeah, I think the spec is a little weird. I agree that it's important for selection and cursor to remain within textareas. I made a note to that effect on the csswg bug. I hadn't thought of contenteditable... probably the same for that. I'll add another note to the csswg bug about this.
We can alter the spec if there's a good reason for it. The last round of discussions concluded hanging the spaces was better for the user because they could see where the caret was in the sequence.
Here are a couple of reftests for the behavior here that fail with current trunk code. The first test passes with the patch above, but the second (which adds text-align:justify) still fails because justification does not behave as expected in the presence of "hanging" spaces at the line break.
Jason, what do you make of a testcase like this?

  data:text/html,<p style="width:0;"><span style="background:cyan;white-space:pre-wrap">a     b</span>

Here, Firefox renders "a" and "b" on successive lines, with the spaces all trailing on the first line. The patch here affects the behavior in that the trailing spaces no longer get the background color, although they're still present (which can be shown by selecting them).

However, both Chrome and Safari show a quite different result: they seem to be putting each of the spaces onto its own line, so that there are a series of blank lines between the "a" and "b".

My reading of the current Editor's Draft suggests that the Chrome/Safari behavior is wrong; do you agree?

What about the rendering of the <span>s background? Is it OK that the trailing spaces no longer get background-painted here?
Flags: needinfo?(jfkthame) → needinfo?(jason)
(In reply to Jonathan Kew (:jfkthame) from comment #47)
> Jason, what do you make of a testcase like this?
> 
>   data:text/html,<p style="width:0;"><span
> style="background:cyan;white-space:pre-wrap">a     b</span>

Oh dear, there are so many weird cases to deal with.


> Here, Firefox renders "a" and "b" on successive lines, with the spaces all
> trailing on the first line. The patch here affects the behavior in that the
> trailing spaces no longer get the background color, although they're still
> present (which can be shown by selecting them).
> 
> 
> 
> However, both Chrome and Safari show a quite different result: they seem to
> be putting each of the spaces onto its own line, so that there are a series
> of blank lines between the "a" and "b".
> 
> My reading of the current Editor's Draft suggests that the Chrome/Safari
> behavior is wrong; do you agree?

I agree. It is my understanding that there is a setting for allowing breaks between spaces, and that this setting is off by default.

I think the spec has only got specific about some of this stuff recently, and it seems like they're still working on some of it.


> What about the rendering of the <span>s background? Is it OK that the
> trailing spaces no longer get background-painted here?

I'm not sure. I'm not sure how this is conceived, or if anybody has thought about this before. Do hanging spaces hang outside the span? At I guess, I suppose the spaces should have the background color. I have no idea how to achieve this though. I hope its okay to do the easy thing for now.


I really hope we can merge a small patch that gets the breaks in the right place before we go too far down this rabbit hole.

Though... I understand that to get the breaks in the right place we have to hang spaces... Oy
Flags: needinfo?(jason)
(In reply to Jason Woofenden from comment #48)

> > What about the rendering of the <span>s background? Is it OK that the
> > trailing spaces no longer get background-painted here?
> 
> I'm not sure. I'm not sure how this is conceived, or if anybody has thought
> about this before. Do hanging spaces hang outside the span? At I guess, I
> suppose the spaces should have the background color.

On re-thinking this, it may not be a problem after all. The reason we don't see the background color of the (hanging) spaces in that example is that their rendering is clipped to the rect of the <p>, which was given zero width. This can be demonstrated, for example, by giving that rect a small but non-zero width such as 20px, and observing that the background of the spaces becomes visible up to that distance.

This is probably a reasonable result (and it matches how Chrome handles it). So I'll take another look at the test failures that led me to this example, and reconsider whether they're legitimate.
(In reply to Jonathan Kew (:jfkthame) from comment #46)
> Created attachment 8782930 [details] [diff] [review]
> Reftests for linebreaking with white-space:pre-wrap and runs of spaces
> 
> Here are a couple of reftests for the behavior here that fail with current
> trunk code. The first test passes with the patch above, but the second
> (which adds text-align:justify) still fails because justification does not
> behave as expected in the presence of "hanging" spaces at the line break.

The second of those tests is not valid, actually. The CSS Text spec allows us to treat white-space:pre-wrap as non-justifiable ("If an element's white space is not collapsible, then the UA is not required to adjust its text for the purpose of justification and may instead treat the text as having no expansion opportunities"), which seems a reasonable thing to do. So I'll drop that testcase from the patch.
Comment on attachment 8756856 [details] [diff] [review]
collapse-ws-at-soft-wrap--v3.patch

Review of attachment 8756856 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I'm happy that this is a good thing to do. It fixes behavior that is currently clearly broken, and brings us substantially closer (at least) to the behavior of other browsers and to the (still-evolving) spec; the patch looks reasonable; and it doesn't cause any new problems that I can see.

We'll need to update a few of our existing tests to account for the change in behavior, but that's fine: the changes are what we'd expect given the new behavior being implemented here. I'll attach a test-fixup patch in a moment. (If we could make this behavior change _without_ needing to adjust tests, that would be a worrying sign that our test coverage is inadequate!)

(Jason, for future reference, when you think a patch is ready to be reviewed and potentially merged, the "proper" way to signal that is to set the r? [review requested] flag on the patch, directed to an appropriate reviewer for the code; that should -- at least in theory -- get the appropriate attention. Here, I'm going to go ahead and just set r+ [review granted] on it, so it's all good. I noticed one minor code style issue, but I'll just fix that locally before landing the patch; no need for extra churn in bugzilla. Oh, and thanks for tackling this, and for your patience through the process!)

::: layout/generic/nsTextFrame.cpp
@@ +8694,5 @@
>    bool canTrimTrailingWhitespace = !textStyle->WhiteSpaceIsSignificant() ||
>                                     (GetStateBits() & TEXT_IS_IN_TOKEN_MATHML);
> +  // allow whitespace to overflow the container
> +  bool whitespaceCanHang = textStyle->WhiteSpaceCanWrapStyle() &&
> +                                textStyle->WhiteSpaceIsSignificant();

style nit: please align 'textStyle' on the wrapped line with the line above
Attachment #8756856 - Flags: review+
Here's a simple reftest for the white-space:pre-wrap line-breaking that's at issue here. It fails with current Gecko trunk code; the patch in this bug will fix it.
Attachment #8784898 - Flags: review?(dholbert)
Attachment #8782930 - Attachment is obsolete: true
And here's the test-update patch that needs to land together with the actual code fix, to update our test expectations. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d34f769ecaaf.
Attachment #8784900 - Flags: review?(dholbert)
Yay! Thank you Jonathan for putting time into this, getting it moving forward and working on tests!

Sorry for the indentation oops, that was probably correct before a variable rename.
Attachment #8784898 - Flags: review?(dholbert) → review+
Comment on attachment 8784900 [details] [diff] [review]
Update reftest and a11y-test expectations to reflect the corrected behavior of white-space:pre-wrap

Review of attachment 8784900 [details] [diff] [review]:
-----------------------------------------------------------------

(Not looking at this in too much detail; seems generally fine. r=me)
Attachment #8784900 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a7bb508cadf8af3384f9e8fb70e5d1dd8ae4ff9
Bug 1008019 - Add reftest (currently fails) for linebreaking with white-space:pre-wrap and runs of spaces. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8917efd53e3a41974fd9ef10d1db3ab1d97f34c
Bug 1008019 - Allow whitespace to "hang" at soft-wrap boundaries when white-space:pre-wrap is in effect. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d87636d76b0a79df4354fc089bf968d0ae9fa39
Bug 1008019 - Update reftest and a11y-test expectations to reflect the corrected behavior of white-space:pre-wrap. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9029f7251f6ed24838905d89ff405f3f72ca428
Backed out changesets 4a7bb508cadf, e8917efd53e3, 2d87636d76b0 (bug 1008019) due to Android reftest orange.
Drat, inbound shows a reftest failure on Android. Looks like a trivial reference-file update needed. I'll fix it and re-land tomorrow.
Assignee: nobody → jason
Flags: needinfo?(dbaron)
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/integration/mozilla-inbound/rev/488e02b69a8c17b7e1b86407cdf340c3129f57eb
Bug 1008019 - Add reftest (currently fails) for linebreaking with white-space:pre-wrap and runs of spaces. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/932c269da17bbf37895bf7f9218d0ebfaed67d38
Bug 1008019 - Allow whitespace to "hang" at soft-wrap boundaries when white-space:pre-wrap is in effect. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/93e9d99aaeadf6fd7693ca9c72f01477ee0e23f8
Bug 1008019 - Update reftest and a11y-test expectations to reflect the corrected behavior of white-space:pre-wrap. r=dholbert
Backed out for failing clipPath-html-06.xhtml:

https://hg.mozilla.org/integration/mozilla-inbound/rev/513b83d59abaf39e1a172198afd3c62d5198575d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1b6e8cd541504fb9ea5c1bff786bc0d865b443
https://hg.mozilla.org/integration/mozilla-inbound/rev/193a64266d1a468d3de906240d301dc49c577cf2

This also happened on the previous landing: https://treeherder.mozilla.org/logviewer.html#?job_id=34693839&repo=mozilla-inbound

Latest push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=93e9d99aaeadf6fd7693ca9c72f01477ee0e23f8
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=34749883&repo=mozilla-inbound

09:02:18     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-06.xhtml == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-06-ref.xhtml | image comparison, max difference: 255, number of differing pixels: 30
Flags: needinfo?(jfkthame)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #60)
> Backed out for failing clipPath-html-06.xhtml:

Sigh..... I saw that on a try push, but it ran green on re-trigger:

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

so I took it to be an unrelated intermittent. And indeed, there are green Linux x64 debug R7 and Ru7 runs on 3742f881a123, which is the push immediately _before_ your backout.

I don't see any reason why the patch here should affect that test. But looking at inbound, it does seem to become a very frequent failure when this is landed. I guess I'll burn some cycles on try, and see if I can narrow down what's going on...
Flags: needinfo?(jfkthame)
OK, I can reproduce the clipPath-html-06 failure locally. It seems to occur because the change to "hanging" spaces at a soft-wrap is affecting the clip-path defined using clipPathUnits="objectBoundingBox". If I alter the testcase by removing the spaces between the <span> and <div> elements that it's actually using, so that there will be no "hanging" whitespaces at the line breaks, the failure goes away.

But this raises a couple of questions: first, is it appropriate for such "hanging" spaces to contribute to the box used in clipPathUnits="objectBoundingBox" calculations? I think probably not. And second, why did the failure not occur consistently across all platforms, and in particular why was it intermittent on some? This suggests a worrying scenario where the behavior is unpredictable (perhaps due to a dependency on an uninitialized value somewhere that affects which "bounding box" gets used?)

I'll try some debugging tools (rr, valgrind, ...) and see if I can pin this down.
This example makes the issue with the patch here easier to see, I think.

Loaded in current Nightly, the second part of the testcase (labelled with Clipped to width="0.5") is clipped so that exactly half its content -- just the left-hand one of each pair of "X"s -- is visible.

But after the above patch is applied, the clipped testcase displays rather more than half of the content; a part of the right-hand "X"s begins to show as well.

This happens because the trailing (hanging) space at the line-break contributes to the bounding box used as a basis for the clipPath units.

(Note that including this space in the visual overflow area of the outer <span> is _beneficial_ in that it means we can correctly display selection highlighting for it -- do a Select All on the testcase to see this. With current Nightly, the selected space/line-break is not visible in any way when doing Select All. So the new behavior is better in this regard, I think.)
On the other hand, here's an example where the patch regresses selection-highlighting behavior, AFAICS:

data:text/html,<style>tt{display:inline-block;width:-moz-min-content;}tt>tt{width:3ch;}</style>
               <tt><tt>a b cd e

This displays the text on three lines:

    a b 
    cd 
    e

If you then do Select All, current Nightly will highlight the text _including_ the spaces between b/c and d/e where it is wrapped. But after the patch is applied, those spaces no longer seem to contribute to the selection highlight.

(Actually, they do -- slightly. If you choose Find and search for a single <space> character, and then click Highlight All, you can see that the line-end spaces are highlighted, but only with a single-pixel line, not the expected character-width rectangle ... whereas in current Nightly, the full rectangles are highlighted.)

I'm not sure why this apparently "reversed" behavior is showing up....
OK, I see why the issue in comment 64 is happening. The reworked measurement in gfxTextRun::BreakAndMeasureText no longer includes the trailing (trimmed) space(s) in the returned bounding-box, because the bbox gets set by the initial measurement that excludes the trailing spaces, and only the width is subsequently updated. To fix this, I've rearranged the code to revert to the previous approach of measuring the full text and then subtracting any trimmed spaces from the width (leaving the full bbox). This restores the previous selection-highlighting in the example above. Jason, could you please confirm that this revised version still gives the desired results for the pre-wrap case that we're trying to fix here? (It looks OK in my testing so far, at least.)
Attachment #8787255 - Flags: feedback?(jason)
Jason, have you had a chance to look at/try out the updated version of the patch here? I was hoping you could confirm that it still fixes the problems for you, before I re-try landing it... thanks!
Flags: needinfo?(jason)
Sorry for being slow. My move to Providence was much harder than it should have been. But we made it!

I should have time tomorrow to get Firefox building again and test the latest patch.
Comment on attachment 8756856 [details] [diff] [review]
collapse-ws-at-soft-wrap--v3.patch

jfkthame's patch is better
Attachment #8756856 - Attachment is obsolete: true
Flags: needinfo?(jason)
I tested your patch (attachment #8787255 [details] [diff] [review]) against my test case attached to this bug, the original jsfiddle, and the html editor I'm working on, and it looks great!

Looks like spaces near breaks are selectable correctly, the breaks happen at the right places, text stays inside textareas, and breaks correctly there too.

Thanks for waiting for me.

In my editor, I flip back and forth between white-space: pre-wrap and normal as needed to preserve spaces. I think this will be much better than adding &nbsp; everywhere, but this makes it important for browsers to break at the same place with pre-wrap. Your patch works great.

What I like about my editor test is that it hits the selection api to find the bounding boxes for each character (for cursor placement). This didn't turn up any anomalies.
Great, thanks for checking it out! (FWIW, I still consider this to be essentially your patch, with review issues tweaked by me. I just rearranged the measurement logic a bit to address the selection-highlighting issue I found, but the key fix here is what you figured out.)
Attachment #8787255 - Flags: feedback?(jason)
https://hg.mozilla.org/integration/mozilla-inbound/rev/50f9f9bec91838935029181f22ae228840b56228
Bug 1008019 - Add reftest (currently fails) for linebreaking with white-space:pre-wrap and runs of spaces. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1901a5332aeb47b3408d6d982f1d9788650812
Bug 1008019 - Allow whitespace to "hang" at soft-wrap boundaries when white-space:pre-wrap is in effect. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/4959bbdb0d99480d556b3738a1bdfb22b371c485
Bug 1008019 - Update reftest and a11y-test expectations to reflect the corrected behavior of white-space:pre-wrap. r=dholbert
Depends on: 1343567
Depends on: 1532520
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: