[web] Position spans absolutely within paragraph #31907
Conversation
|
Gold has detected about 5 new digest(s) on patchset 1. |
|
Gold has detected about 5 new digest(s) on patchset 2. |
|
Gold has detected about 5 new digest(s) on patchset 3. |
|
Gold has detected about 23 new digest(s) on patchset 5. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
| @@ -307,6 +266,9 @@ void _applyNecessaryParagraphStyles({ | |||
| }) { | |||
| final html.CssStyleDeclaration cssStyle = element.style; | |||
|
|
|||
| // TODO(mdebbar): Now that we absolutely position each span inside the | |||
| // paragraph, do we still need these style on <p>? | |||
There was a problem hiding this comment.
Do we cascade the styles down to span level? In Flutter, paragraph styles and parent text styles are meant to be "inherited" to descendants.
There was a problem hiding this comment.
As discussed offline, the classes RootStyleNode and ChildStyleNode handle the inheritance of styles.
| /// info in the following tests only pass in Chrome, they are slightly different | ||
| /// on each browser. So we need to ignore position info on non-Chrome browsers | ||
| /// when comparing expectations with actual output. | ||
| bool get isChrome => browserEngine == BrowserEngine.blink; |
There was a problem hiding this comment.
nit: how about isBlink to stress that this applies to all Blink-based browsers?
| @@ -421,3 +493,23 @@ TextStyle styleWithDefaults({ | |||
| letterSpacing: letterSpacing, | |||
| ); | |||
| } | |||
|
|
|||
| void expectOuterHtml(CanvasParagraph paragraph, String expected, {required bool ignorePositions}) { | |||
There was a problem hiding this comment.
FYI: we also have expectHtml in test/matchers.dart, but I'm not sure if it's useful here.
|
|
||
| if (box is SpanBox) { | ||
| span = box.span; | ||
| element = html.document.createElement('span') as html.HtmlElement; | ||
| lastSpanElement = html.document.createElement('span') as html.HtmlElement; |
There was a problem hiding this comment.
Why not html.SpanElement()?
There was a problem hiding this comment.
No reason. Is one different from the other?
I'll keep it as html.document.createElement('span') for now so I can easily change it to flt-span next.
|
I just confirmed that this PR fixes the Chinese text issue (flutter/flutter#99794).
|
|
Gold has detected about 18 new digest(s) on patchset 6. |
|
Golden file changes are available for triage from new commit, Click here to view. |
mdebbar
added
the
waiting for tree to go green
* 9f13454 [windows] Support win_debug_x86 compilation target in engine (flutter/engine#30417) * c5e958e Make tracing safe (flutter/engine#32042) * cf875d4 [macOS] Forward key events to NSTextInputContext when there's composing text (flutter/engine#32051) * ffd5c9c Roll Fuchsia Linux SDK from Ee9OX2o6P... to mVqiTwaVa... (flutter/engine#32055) * b0018c3 Roll Skia from a9bc6c643791 to 8afe53fd76d3 (1 revision) (flutter/engine#32056) * df12321 Roll Skia from 8afe53fd76d3 to 0d81bc7bb04e (2 revisions) (flutter/engine#32057) * 398a274 Roll Fuchsia Mac SDK from jvlI1s78T... to vWlaMIVkM... (flutter/engine#32060) * 3ffa9cf Roll Skia from 0d81bc7bb04e to e253cc3e55d3 (1 revision) (flutter/engine#32063) * 275cd2b [web] Position spans absolutely within paragraph (flutter/engine#31907) * 46c2cca Roll Skia from e253cc3e55d3 to 9301fe3779bb (1 revision) (flutter/engine#32064)
We previously let HTML's layout control the flow of spans and placeholders inside a paragraph. We just set enough styles to make the browser lay them out the way we want.
This proved challenging, and there were always minor discrepancies between paragraphs rendered to the DOM vs canvas2d. E.g. trailing white spaces, placeholder alignment, etc.
One of the issues that we need to fix is
TextAlign.justify(flutter/flutter#96533), so it's a good time to try and address the above-mentioned issues as well with the same solution.The way we are fixing this is by absolutely positioning each span inside the paragraph based on the results of our layout results.
Fixes flutter/flutter#99794
Fixes flutter/flutter#64792