|
Description
Javier Fernandez
2017-09-21 15:39:20 PDT
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. (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. (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? (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. (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! Created attachment 340493 [details]
Patch
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.
@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. (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. 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. 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. 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. 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. We should also make sure the minimum and maximum intrinsic widths are correctly affected (or not affected) (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. 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 Created attachment 344840 [details]
Patch
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 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
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 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
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 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
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 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
Created attachment 366339 [details]
Patch
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 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
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 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
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 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
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 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
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 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
Created attachment 366425 [details]
Patch
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 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
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 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
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 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
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 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
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 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
Created attachment 366484 [details]
Patch
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 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
Created attachment 366495 [details]
Patch
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 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
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. 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 Created attachment 366596 [details]
Patch
(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. 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 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
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. (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; Created attachment 366925 [details]
Patch
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 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
Created attachment 366931 [details]
Patch
Comment on attachment 366931 [details] Patch Clearing flags on attachment: 366931 Committed r244036: <https://trac.webkit.org/changeset/244036> All reviewed patches have been landed. Closing bug. |