Bug 177327 – Implement white-space:break-spaces value

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Javier Fernandez]

The CSS Text 3 spec defines the 'overflow-wrap' property as property:

https://drafts.csswg.org/css-text-3/#overflow-wrap-property

"Value: 	normal | break-word || break-spaces"

[Javier Fernandez]

I'm asking Blink and Gecko engineers about the plans to implement this feature. It'd be great to have an idea about the WebKit position and its level of support to implement this.

[Myles C. Maxfield]

(In reply to Javier Fernandez from comment #1)
> I'm asking Blink and Gecko engineers about the plans to implement this
> feature. It'd be great to have an idea about the WebKit position and its
> level of support to implement this.

It would be really great if we could rely on ICU more to handle more cases of line breaking. As I understand it, ICU aspires to handle all the line-breaking situations that are describable in CSS. Given this, I think we'd be happy to pass more information to ICU to describe the environment about how to do line breaking. If ICU doesn't currently support overflow-wrap:break-spaces behavior, we shouldn't implement it in WebKit; we should instead either wait for ICU or specifically ask ICU to support it. If ICU thinks that they will never support overflow-wrap:break-spaces behavior, we should push against it in the W3C.

Regardless of what ICU's stance or current support level is, we should start on the task of deleting more of WebKit's line breaking code in favor of using ICU. This involves a few different tasks:
1) Discovering which parts of CSS-Text ICU already supports
2) Measuring the performance of their line breaking algorithms, and, if necessary implement some system (caching?) to try to speed it up.

This project is something I'd like to do in the coming years.

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #2)
> (In reply to Javier Fernandez from comment #1)
> > I'm asking Blink and Gecko engineers about the plans to implement this
> > feature. It'd be great to have an idea about the WebKit position and its
> > level of support to implement this.
> 
> It would be really great if we could rely on ICU more to handle more cases
> of line breaking. As I understand it, ICU aspires to handle all the
> line-breaking situations that are describable in CSS. Given this, I think
> we'd be happy to pass more information to ICU to describe the environment
> about how to do line breaking. If ICU doesn't currently support
> overflow-wrap:break-spaces behavior, we shouldn't implement it in WebKit; we
> should instead either wait for ICU or specifically ask ICU to support it. If
> ICU thinks that they will never support overflow-wrap:break-spaces behavior,
> we should push against it in the W3C.
> 
> Regardless of what ICU's stance or current support level is, we should start
> on the task of deleting more of WebKit's line breaking code in favor of
> using ICU. This involves a few different tasks:
> 1) Discovering which parts of CSS-Text ICU already supports
> 2) Measuring the performance of their line breaking algorithms, and, if
> necessary implement some system (caching?) to try to speed it up.
> 
> This project is something I'd like to do in the coming years.

I started looking at this more deeply, and I realized you are talking about a different property than the one I thought you were talking about. overflow-wrap doesn't create a new pattern of line-breaking opportunities; instead it says "if you didn't find any opportunities using one kind of iterator, use another kind of iterator." The novel piece here is the handoff from one iterator to another iterator; ICU already implements both kinds of iterators correctly. Therefore, we don't need to get involved with ICU for this feature. Sorry for the misdirection.

The break-spaces value of overflow-wrap only interacts with whitespace, and because there are only a few whitespace characters, handling this logic directly inside WebKit seems fine. You could probably do this quickly by hacking on BreakingContext::handleText() (and either opting out of simple line layout or implementing it there too). However, this function is probably the least maintainable function relating to text in the entire codebase, so rather than implementing support directly here, a better approach would be to put the implementation behind the TextBreakIterator API, and to try to implement the kind of handoff structure that we could use for overflow-wrap: break-word. Eventually, overflow-wrap: break-word and word-break: break-all should use the same grapheme-cluster-based iterator (and overflow-wrap: break-word would hand-off between this and a regular iterator, where word-break: break-all wouldn't).

This would be a fantastic first step toward the long-term plan to make BreakingContext::handleText() more understandable and maintainable.

Does this sound okay? What do you think about it?

[Javier Fernandez]

(In reply to Myles C. Maxfield from comment #3)
> (In reply to Myles C. Maxfield from comment #2)
> > (In reply to Javier Fernandez from comment #1)
> > > I'm asking Blink and Gecko engineers about the plans to implement this
> > > feature. It'd be great to have an idea about the WebKit position and its
> > > level of support to implement this.
> > 
> > It would be really great if we could rely on ICU more to handle more cases
> > of line breaking. As I understand it, ICU aspires to handle all the
> > line-breaking situations that are describable in CSS. Given this, I think
> > we'd be happy to pass more information to ICU to describe the environment
> > about how to do line breaking. If ICU doesn't currently support
> > overflow-wrap:break-spaces behavior, we shouldn't implement it in WebKit; we
> > should instead either wait for ICU or specifically ask ICU to support it. If
> > ICU thinks that they will never support overflow-wrap:break-spaces behavior,
> > we should push against it in the W3C.
> > 
> > Regardless of what ICU's stance or current support level is, we should start
> > on the task of deleting more of WebKit's line breaking code in favor of
> > using ICU. This involves a few different tasks:
> > 1) Discovering which parts of CSS-Text ICU already supports
> > 2) Measuring the performance of their line breaking algorithms, and, if
> > necessary implement some system (caching?) to try to speed it up.
> > 
> > This project is something I'd like to do in the coming years.
> 
> I started looking at this more deeply, and I realized you are talking about
> a different property than the one I thought you were talking about.
> overflow-wrap doesn't create a new pattern of line-breaking opportunities;
> instead it says "if you didn't find any opportunities using one kind of
> iterator, use another kind of iterator." The novel piece here is the handoff
> from one iterator to another iterator; ICU already implements both kinds of
> iterators correctly. Therefore, we don't need to get involved with ICU for
> this feature. Sorry for the misdirection.
> 
> The break-spaces value of overflow-wrap only interacts with whitespace, and
> because there are only a few whitespace characters, handling this logic
> directly inside WebKit seems fine. You could probably do this quickly by
> hacking on BreakingContext::handleText() (and either opting out of simple
> line layout or implementing it there too). However, this function is
> probably the least maintainable function relating to text in the entire
> codebase, so rather than implementing support directly here, a better
> approach would be to put the implementation behind the TextBreakIterator
> API, and to try to implement the kind of handoff structure that we could use
> for overflow-wrap: break-word. Eventually, overflow-wrap: break-word and
> word-break: break-all should use the same grapheme-cluster-based iterator
> (and overflow-wrap: break-word would hand-off between this and a regular
> iterator, where word-break: break-all wouldn't).
> 
> This would be a fantastic first step toward the long-term plan to make
> BreakingContext::handleText() more understandable and maintainable.
> 
> Does this sound okay? What do you think about it?

Sorry for the very late reply. I've been waiting for the spec to stabilize and it seems that after the F2F in Berlin it's in a good state now. 

I'll start working on this now, but I still have to learn about this new codebase and the spec, which I'm not familiar with.

[Myles C. Maxfield]

(In reply to Javier Fernandez from comment #4)
> (In reply to Myles C. Maxfield from comment #3)
> > (In reply to Myles C. Maxfield from comment #2)
> > > (In reply to Javier Fernandez from comment #1)
> > > > I'm asking Blink and Gecko engineers about the plans to implement this
> > > > feature. It'd be great to have an idea about the WebKit position and its
> > > > level of support to implement this.
> > > 
> > > It would be really great if we could rely on ICU more to handle more cases
> > > of line breaking. As I understand it, ICU aspires to handle all the
> > > line-breaking situations that are describable in CSS. Given this, I think
> > > we'd be happy to pass more information to ICU to describe the environment
> > > about how to do line breaking. If ICU doesn't currently support
> > > overflow-wrap:break-spaces behavior, we shouldn't implement it in WebKit; we
> > > should instead either wait for ICU or specifically ask ICU to support it. If
> > > ICU thinks that they will never support overflow-wrap:break-spaces behavior,
> > > we should push against it in the W3C.
> > > 
> > > Regardless of what ICU's stance or current support level is, we should start
> > > on the task of deleting more of WebKit's line breaking code in favor of
> > > using ICU. This involves a few different tasks:
> > > 1) Discovering which parts of CSS-Text ICU already supports
> > > 2) Measuring the performance of their line breaking algorithms, and, if
> > > necessary implement some system (caching?) to try to speed it up.
> > > 
> > > This project is something I'd like to do in the coming years.
> > 
> > I started looking at this more deeply, and I realized you are talking about
> > a different property than the one I thought you were talking about.
> > overflow-wrap doesn't create a new pattern of line-breaking opportunities;
> > instead it says "if you didn't find any opportunities using one kind of
> > iterator, use another kind of iterator." The novel piece here is the handoff
> > from one iterator to another iterator; ICU already implements both kinds of
> > iterators correctly. Therefore, we don't need to get involved with ICU for
> > this feature. Sorry for the misdirection.
> > 
> > The break-spaces value of overflow-wrap only interacts with whitespace, and
> > because there are only a few whitespace characters, handling this logic
> > directly inside WebKit seems fine. You could probably do this quickly by
> > hacking on BreakingContext::handleText() (and either opting out of simple
> > line layout or implementing it there too). However, this function is
> > probably the least maintainable function relating to text in the entire
> > codebase, so rather than implementing support directly here, a better
> > approach would be to put the implementation behind the TextBreakIterator
> > API, and to try to implement the kind of handoff structure that we could use
> > for overflow-wrap: break-word. Eventually, overflow-wrap: break-word and
> > word-break: break-all should use the same grapheme-cluster-based iterator
> > (and overflow-wrap: break-word would hand-off between this and a regular
> > iterator, where word-break: break-all wouldn't).
> > 
> > This would be a fantastic first step toward the long-term plan to make
> > BreakingContext::handleText() more understandable and maintainable.
> > 
> > Does this sound okay? What do you think about it?
> 
> Sorry for the very late reply. I've been waiting for the spec to stabilize
> and it seems that after the F2F in Berlin it's in a good state now. 
> 
> I'll start working on this now, but I still have to learn about this new
> codebase and the spec, which I'm not familiar with.

Okay. Please let me know how I can help. I'm very interested in this!

[Javier Fernandez]

Created attachment 340493 [details]
Patch

[Javier Fernandez]

The patch in attachment #340493 [details] is a very preliminary approach to implement the new 'break-spaces' value for the 'overflow-wrap' CSS property. 

The parsing logic is still uncompleted, since the new syntax allow a combination of 'break-word' and 'break-spaces' values (in any oder), but while still discussing about the best approach to implement this, I'd rather keep it simple. 

So, I've spent a few days trying to understand the codebase involved in inline-level boxes layout and the line-braking logic. I think I've got an overall idea of the general design and the classes involved. However, I still have doubts about where to implement the new line-breaking features.

As far I understand the spec [1] and the prose about the new value, the following points are the ones supporting my current (preliminary) approach. 

The new 'break-spaces' value does not introduce new breaking
opportunities, but just forbids collapsing [2] to let the current
line-breaking logic to handle those spaces as preserved white-spaces:

"However, if overflow-wrap is set to break-spaces, collapsing their
advance width is not allowed, as this would prevent the preserved
spaces from wrapping. "

Additionally, it introduces some restrictions [3] to where this
preserved sequence of white spaces can be broken:

" ... after the last white space character that would fit the line, or
after the first white space in the sequence if none would fit, or
before the first space in the sequence if none would fit and both
break-word and break-spaces are specified."

Comments and feedback are really welcome.

[Javier Fernandez]

@Myles could you add some feedback about the general design of the patch ? I'm already discussing these details in the Blink CL, but perhaps we can share some ideas here as well. 

The patch is in a very early state, specially regarding the parsing logic, which for now only parses single values. However, I'd like to focus now on the new 'break-spaces' value alone; how it avoid the white-spaces to collapse, whether it introduces new breaking opportunities (or forbid ones already implemented) and where (which class/function) these should be implemented: LazyLineBreakIterator(ICU) or LineBreaker/BrakingContext.

[Myles C. Maxfield]

(In reply to Javier Fernandez from comment #8)
> @Myles could you add some feedback about the general design of the patch ?
> I'm already discussing these details in the Blink CL, but perhaps we can
> share some ideas here as well. 
> 
> The patch is in a very early state, specially regarding the parsing logic,
> which for now only parses single values. However, I'd like to focus now on
> the new 'break-spaces' value alone; how it avoid the white-spaces to
> collapse, whether it introduces new breaking opportunities (or forbid ones
> already implemented) and where (which class/function) these should be
> implemented: LazyLineBreakIterator(ICU) or LineBreaker/BrakingContext.

Yeah, I'll look at it today.

[Myles C. Maxfield]

Comment on attachment 340493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340493&action=review

I think this is a fine direction to start with. How much more complicated do you think the full feature will be? If it's not super complicated, we can probably just keep hacking on BreakingContext.h, but if this turns too complicated, we probably want to rip up BreakingContext.h together and redesign it.

How comprehensive are the WPT tests for this?

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:606
> +        return valueID == CSSValueNormal || valueID == CSSValueBreakWord || valueID == CSSValueBreakSpaces;

https://drafts.csswg.org/css-text-3/#propdef-word-wrap says that the syntax is normal | break-word || break-spaces

> Source/WebCore/rendering/line/BreakingContext.h:985
> +                        if (m_currentCharacterIsSpace && !previousCharacterIsSpace && !m_currentStyle->canBreakBeforeFirstWhiteSpace())
> +                            return false;

I think this is fine for now, but is there a more elegant way we can do this?

I don't know, this whole function is smelly.

> Source/WebCore/rendering/line/LineInlineHeaders.h:71
> +        || (whitespacePosition == TrailingWhitespace && style->whiteSpace() == PRE_WRAP && style->overflowWrap() != BreakSpacesOverflowWrap && (!lineInfo.isEmpty() || !lineInfo.previousLineBrokeCleanly()));

I don't think breaking spaces controls collapsing. I think it means "if whitespace isn't collapsed, then a sequence of spaces can have breaking opportunities within the sequence of whitespace."

Unless there is some part of the spec I don't understand?

> Source/WebCore/rendering/style/RenderStyleConstants.h:311
> +    NormalOverflowWrap, BreakOverflowWrap, BreakSpacesOverflowWrap

Because of the syntax, this has to be a bitfield.

[Javier Fernandez]

Thanks for taking the time to review this, despite the early stage of the patch.

(In reply to Myles C. Maxfield from comment #10)
> Comment on attachment 340493 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340493&action=review
> 
> I think this is a fine direction to start with. How much more complicated do
> you think the full feature will be? If it's not super complicated, we can
> probably just keep hacking on BreakingContext.h, but if this turns too
> complicated, we probably want to rip up BreakingContext.h together and
> redesign it.
> 

I think that's the key point. My impression is that the feature is relatively simple, as it doesn't introduce new breaking opportunities, but limiting some of the ones we already have implemented in other line-breaking properties. There was some discussion [1] about the combination 'break-word break-spaces', which would allow breaking the white-spaces sequence before the first space, but I don't think it'll increase the complexity too much.

However, during the discussion in the blink CL, there is always the request to move any new braking opportunity to the ICU implementation, which if I understood it correctly, relies on the LazyLineBrakingIterator. We can follow a similar approach eventually, but I think we can start trying to implement it via smalls changes in the BrakingContext class.  

> How comprehensive are the WPT tests for this?
> 

I think that Forian did a good initial effort in the test suite and there are a around 20 or 30 tests to verify the basic behavior of the break-spaces value and the combination with the break-word value. There are also tests to verify how it behaves when combined with the white-space property, mainly with the pre-wrap value, which I think it's one of the main use cases for this new value. 

However, and this is also a concern raised during the blink CL discussion, the test suite lack of tests with other combinations of line-braking properties, as 'line-break: anywhere' or break-word: break-all. 

Anyway, Florian commented about his intention to continue working on adding tests and I'll also devote a considerable part of my work to this task.

[Javier Fernandez]

Comment on attachment 340493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340493&action=review

>> Source/WebCore/css/parser/CSSParserFastPaths.cpp:606
>> +        return valueID == CSSValueNormal || valueID == CSSValueBreakWord || valueID == CSSValueBreakSpaces;
> 
> https://drafts.csswg.org/css-text-3/#propdef-word-wrap says that the syntax is normal | break-word || break-spaces

Yes, I preferred to keep the simple CSS syntax for now, to focus the discussion on the line-breaking logic and figure out whether it's enough to touch the BrakingContext class or we'll need a new design. 

Anyway, I'm already working on a patch to implement the new parsing logic to allow complex values.

>> Source/WebCore/rendering/line/BreakingContext.h:985
>> +                            return false;
> 
> I think this is fine for now, but is there a more elegant way we can do this?
> 
> I don't know, this whole function is smelly.

I agree with you that this should indeed be a temporary change. I'm still trying to understand the whole logic, but this change helped me to point out that this new value should forbid breaking at the first white space, unless break-word is used in combination with break-spaces. I'll think about better ways to implement it.

>> Source/WebCore/rendering/line/LineInlineHeaders.h:71
>> +        || (whitespacePosition == TrailingWhitespace && style->whiteSpace() == PRE_WRAP && style->overflowWrap() != BreakSpacesOverflowWrap && (!lineInfo.isEmpty() || !lineInfo.previousLineBrokeCleanly()));
> 
> I don't think breaking spaces controls collapsing. I think it means "if whitespace isn't collapsed, then a sequence of spaces can have breaking opportunities within the sequence of whitespace."
> 
> Unless there is some part of the spec I don't understand?

My understanding is that while it's true that break-spaces doesn't control spaces collapsing, it indeed prevents it in some situations:

https://drafts.csswg.org/css-text-3/#white-space-phase-2

4. If spaces or tabs at the end of a line are non-collapsible but have white-space set to pre-wrap:

    * If overflow-wrap does not specify break-spaces the UA must either hang the white space or visually collapse the character advance widths of any overflowing spaces such that they don’t take up space in the line.
    * Otherwise (i.e. overflow-wrap property specifies break-spaces), hanging or collapsing the advance width of spaces at the end of the line is not allowed.

>> Source/WebCore/rendering/style/RenderStyleConstants.h:311
>> +    NormalOverflowWrap, BreakOverflowWrap, BreakSpacesOverflowWrap
> 
> Because of the syntax, this has to be a bitfield.

Indeed, but let's leaving aside this until I'll provide a patch with the new CSS parsing logic.

[Javier Fernandez]

I forgot to add the mentioned link to the discussion, which you started some time ago :) 

https://github.com/w3c/csswg-drafts/issues/2003

(In reply to Javier Fernandez from comment #11)
> Thanks for taking the time to review this, despite the early stage of the
> patch.
> 
> (In reply to Myles C. Maxfield from comment #10)
> > Comment on attachment 340493 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=340493&action=review
> > 
> > I think this is a fine direction to start with. How much more complicated do
> > you think the full feature will be? If it's not super complicated, we can
> > probably just keep hacking on BreakingContext.h, but if this turns too
> > complicated, we probably want to rip up BreakingContext.h together and
> > redesign it.
> > 
> 
> I think that's the key point. My impression is that the feature is
> relatively simple, as it doesn't introduce new breaking opportunities, but
> limiting some of the ones we already have implemented in other line-breaking
> properties. There was some discussion [1] about the combination 'break-word
> break-spaces', which would allow breaking the white-spaces sequence before
> the first space, but I don't think it'll increase the complexity too much.

[Myles C. Maxfield]

We should also make sure the minimum and maximum intrinsic widths are correctly affected (or not affected)

[Myles C. Maxfield]

(In reply to Javier Fernandez from comment #11)
> Thanks for taking the time to review this, despite the early stage of the
> patch.
> 
> (In reply to Myles C. Maxfield from comment #10)
> > Comment on attachment 340493 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=340493&action=review
> > 
> > I think this is a fine direction to start with. How much more complicated do
> > you think the full feature will be? If it's not super complicated, we can
> > probably just keep hacking on BreakingContext.h, but if this turns too
> > complicated, we probably want to rip up BreakingContext.h together and
> > redesign it.
> > 
> 
> I think that's the key point. My impression is that the feature is
> relatively simple, as it doesn't introduce new breaking opportunities, but
> limiting some of the ones we already have implemented in other line-breaking
> properties. There was some discussion [1] about the combination 'break-word
> break-spaces', which would allow breaking the white-spaces sequence before
> the first space, but I don't think it'll increase the complexity too much.

Okay, if you're comfortable with this direction, then it seems fine to me.

> 
> However, during the discussion in the blink CL, there is always the request
> to move any new braking opportunity to the ICU implementation

Yeah, this is what my comment in https://bugs.webkit.org/show_bug.cgi?id=177327#c3 is about. This property is about the interaction between two types of line breaking, both of which already exist in ICU. Therefore, I don't think ICU is the right place to implement this.

> , which if I
> understood it correctly, relies on the LazyLineBrakingIterator. We can
> follow a similar approach eventually, but I think we can start trying to
> implement it via smalls changes in the BrakingContext class.

Right. LazyLineBreakIterator conceptually represents an ICU UBreakIterator. BreakingContext uses the lower-level breaking iterators. Because break-spaces is about the relationship between two different breaking iterators, it belongs in the higher level.

> 
> > How comprehensive are the WPT tests for this?
> > 
> 
> I think that Forian did a good initial effort in the test suite and there
> are a around 20 or 30 tests to verify the basic behavior of the break-spaces
> value and the combination with the break-word value. There are also tests to
> verify how it behaves when combined with the white-space property, mainly
> with the pre-wrap value, which I think it's one of the main use cases for
> this new value. 
> 
> However, and this is also a concern raised during the blink CL discussion,
> the test suite lack of tests with other combinations of line-braking
> properties, as 'line-break: anywhere' or break-word: break-all. 
> 
> Anyway, Florian commented about his intention to continue working on adding
> tests and I'll also devote a considerable part of my work to this task.

[Javier Fernandez]

The CSS WG has resolved [1] that the new 'break-spaces' value should belong to the 'white-space' property, instead of 'overflow-wrap'. 

[1]| https://github.com/w3c/csswg-drafts/issues/2465#issuecomment-400757345

[Javier Fernandez]

Created attachment 344840 [details]
Patch

[EWS Watchlist]

Comment on attachment 344840 [details]
Patch

Attachment 344840 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8515246

New failing tests:
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-010.html
imported/w3c/web-platform-tests/css/css-text/white-space/break-spaces-001.html
imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html
imported/w3c/web-platform-tests/css/css-text/white-space/trailing-space-before-br-001.html
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-008.html
imported/w3c/web-platform-tests/css/css-text/white-space/pre-wrap-010.html

[EWS Watchlist]

Created attachment 344844 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344840 [details]
Patch

Attachment 344840 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8515356

New failing tests:
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-010.html
imported/w3c/web-platform-tests/css/css-text/white-space/break-spaces-001.html
imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html
imported/w3c/web-platform-tests/css/css-text/white-space/trailing-space-before-br-001.html
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-008.html
imported/w3c/web-platform-tests/css/css-text/white-space/pre-wrap-010.html

[EWS Watchlist]

Created attachment 344846 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344840 [details]
Patch

Attachment 344840 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8515380

New failing tests:
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-010.html
imported/w3c/web-platform-tests/css/css-text/white-space/break-spaces-001.html
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-009.html
imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html
imported/w3c/web-platform-tests/css/css-text/white-space/trailing-space-before-br-001.html
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-008.html
imported/w3c/web-platform-tests/css/css-text/white-space/pre-wrap-010.html

[EWS Watchlist]

Created attachment 344847 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 344840 [details]
Patch

Attachment 344840 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8516009

New failing tests:
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-010.html
imported/w3c/web-platform-tests/css/css-text/white-space/break-spaces-001.html
imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html
imported/w3c/web-platform-tests/css/css-text/white-space/trailing-space-before-br-001.html
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-pre-wrap-008.html
imported/w3c/web-platform-tests/css/css-text/white-space/pre-wrap-010.html

[EWS Watchlist]

Created attachment 344852 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Javier Fernandez]

Created attachment 366339 [details]
Patch

[EWS Watchlist]

Comment on attachment 366339 [details]
Patch

Attachment 366339 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11709147

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
editing/deleting/delete-block-table.html
fast/css/font-face-zero-hash-key.html

[EWS Watchlist]

Created attachment 366343 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366339 [details]
Patch

Attachment 366339 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11709112

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
editing/deleting/delete-block-table.html
fast/css/font-face-zero-hash-key.html

[EWS Watchlist]

Created attachment 366344 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366339 [details]
Patch

Attachment 366339 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11709686

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
fast/css/font-face-zero-hash-key.html
editing/deleting/delete-block-table.html

[EWS Watchlist]

Created attachment 366354 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 366339 [details]
Patch

Attachment 366339 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11709846

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
editing/deleting/delete-block-table.html
fast/css/font-face-zero-hash-key.html

[EWS Watchlist]

Created attachment 366355 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366339 [details]
Patch

Attachment 366339 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11710083

New failing tests:
editing/deleting/delete-to-select-table.html
editing/style/table-selection.html
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-break-spaces-001.html
editing/deleting/delete-block-table.html
fast/inline/trailing-floats-inline-crash.html
fast/inline/crash-when-inline-box-has-invalid-float.html
fast/css/font-face-zero-hash-key.html

[EWS Watchlist]

Created attachment 366358 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Javier Fernandez]

Created attachment 366425 [details]
Patch

[EWS Watchlist]

Comment on attachment 366425 [details]
Patch

Attachment 366425 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11731921

New failing tests:
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
fast/css/font-face-zero-hash-key.html

[EWS Watchlist]

Created attachment 366433 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366425 [details]
Patch

Attachment 366425 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11731888

New failing tests:
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
fast/css/font-face-zero-hash-key.html

[EWS Watchlist]

Created attachment 366434 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366425 [details]
Patch

Attachment 366425 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11732018

New failing tests:
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
fast/css/font-face-zero-hash-key.html

[EWS Watchlist]

Created attachment 366441 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366425 [details]
Patch

Attachment 366425 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11732232

New failing tests:
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
fast/css/font-face-zero-hash-key.html

[EWS Watchlist]

Created attachment 366444 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 366425 [details]
Patch

Attachment 366425 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11732221

New failing tests:
fast/text/simple-line-layout-multiple-trailingwhitespace-crash.html
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-break-spaces-001.html
fast/css/font-face-zero-hash-key.html
imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html

[EWS Watchlist]

Created attachment 366446 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Javier Fernandez]

Created attachment 366484 [details]
Patch

[EWS Watchlist]

Comment on attachment 366484 [details]
Patch

Attachment 366484 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11738413

New failing tests:
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-break-spaces-001.html
imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html

[EWS Watchlist]

Created attachment 366490 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Javier Fernandez]

Created attachment 366495 [details]
Patch

[EWS Watchlist]

Comment on attachment 366495 [details]
Patch

Attachment 366495 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11740022

New failing tests:
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-break-spaces-001.html

[EWS Watchlist]

Created attachment 366500 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

Comment on attachment 366495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366495&action=review

r+ but Alan should look at the SimpleLineLayout changes.

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:730
> +    case CSSPropertyLineBreak: // auto | loose | normal | strict | after-white-space | break-spacess

s/break-spacess/break-spaces/

Also, shouldn't this new value be on "white-space" instead of "line-break"?

> LayoutTests/platform/ios/TestExpectations:3061
> +webkit.org/b/196169 imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html [ ImageOnlyFailure ]

This is surprising, and I don't see anything in the ChangeLog about it.

> LayoutTests/platform/mac/TestExpectations:1690
> +webkit.org/b/196169 imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html [ ImageOnlyFailure ]

Ditto.

[Javier Fernandez]

Comment on attachment 366495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366495&action=review

>> Source/WebCore/css/parser/CSSParserFastPaths.cpp:730
>> +    case CSSPropertyLineBreak: // auto | loose | normal | strict | after-white-space | break-spacess
> 
> s/break-spacess/break-spaces/
> 
> Also, shouldn't this new value be on "white-space" instead of "line-break"?

Opps, notice that it's just a comment, which is in the wrong place in any case. The actual new value is added in the 'white-space' property.

>> LayoutTests/platform/ios/TestExpectations:3061
>> +webkit.org/b/196169 imported/w3c/web-platform-tests/css/css-text/overflow-wrap/overflow-wrap-break-word-003.html [ ImageOnlyFailure ]
> 
> This is surprising, and I don't see anything in the ChangeLog about it.

Well, the bug is pretty clear, I'd say. Actually, I had to send a PR [1] to change some tests to use Ahem instead of monospace precisely because bug #196353 which I think it's the root cause of webkit.org/b/196169. 
Additionally, I created another PR [2] to define two new tests, identical to this one failing in mac and ios platforms, but using the Ahem font.  

However, it's true that I should have add some lines to the ChangeLog explaining the issue.


[1] https://github.com/web-platform-tests/wpt/pull/16137
[2] https://github.com/web-platform-tests/wpt/pull/16124

[Javier Fernandez]

Created attachment 366596 [details]
Patch

[Javier Fernandez]

(In reply to Build Bot from comment #52)
> Comment on attachment 366495 [details]
> Patch
> 
> Attachment 366495 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/11740022
> 
> New failing tests:
> imported/w3c/web-platform-tests/css/css-text/white-space/textarea-break-
> spaces-001.html

I'm not sure how to get rid of this test failure. It seems a weird rendering of the expected file, which instead of a perfect green square it shows the left corners slightly round.

[EWS Watchlist]

Comment on attachment 366596 [details]
Patch

Attachment 366596 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11751207

New failing tests:
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-break-spaces-001.html

[EWS Watchlist]

Created attachment 366609 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Javier Fernandez]

Hi @zalan, could you please take a look at this patch ? Specially to the changes in the SimpleLineLayout class. Also, if you could give me some advice on how to deal with the iOS regression it'd be really helpful.

[zalan]

(In reply to Javier Fernandez from comment #60)
> Hi @zalan, could you please take a look at this patch ? Specially to the
> changes in the SimpleLineLayout class. Also, if you could give me some
> advice on how to deal with the iOS regression it'd be really helpful.
Not sure why that's there but it clearly comes from html.css. I'd just fix it with border-radius: 0px;

[Javier Fernandez]

Created attachment 366925 [details]
Patch

[EWS Watchlist]

Comment on attachment 366925 [details]
Patch

Attachment 366925 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11804763

New failing tests:
imported/w3c/web-platform-tests/css/css-text/white-space/textarea-break-spaces-001.html

[EWS Watchlist]

Created attachment 366930 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Javier Fernandez]

Created attachment 366931 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 366931 [details]
Patch

Clearing flags on attachment: 366931

Committed r244036: <https://trac.webkit.org/changeset/244036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49708962>