Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817443003:
Fix font-size shaking issue in some pages (Closed)
|
Created:
3 years, 8 months ago by cathiechentx Modified:
3 years, 6 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix font-size shaking issue in some page
In some page content is autosized based on multiplier of LayoutView.
In order to make the multiplier of LayoutView more steady,
use LayoutView itself as widthProvider instead of finding
deepestBlockContainingAllText.
BUG=452709
Review-Url: https://codereview.chromium.org/2817443003
Cr-Commit-Position: refs/heads/master@{#477198}
Committed: https://chromium.googlesource.com/chromium/src/+/47f9b0ff7d4594f6633956ac570966439b4d5125
Patch Set 1 #
Total comments: 1
Patch Set 2 : update comment #Patch Set 3 : rebase #Patch Set 4 : layout test failed #
Total comments: 3
Patch Set 5 : rebaseline layout-after-append.html #
Total comments: 7
Patch Set 6 : fix font-size shaking issue only #
Total comments: 5
Patch Set 7 : remove root check #Patch Set 8 : update code and remove TestExpectations rebaseline #Patch Set 9 : rebase layout-after-append.html #Messages
Total messages: 43 (20 generated)
Description was changed from ========== Fix font-size shaking issue in some pages In some page content text is autosized base on multiplier of LayoutView. In order to make the multiplier of LayoutView steady, use LayoutView itself as widthProvider instead of finding deepestBlockContainingAllText. BUG=452709 ========== to ========== Fix font-size shaking issue in some pages In some page content text is autosized based on multiplier of LayoutView. In order to make the multiplier of LayoutView more steady, use LayoutView itself as widthProvider instead of finding deepestBlockContainingAllText. BUG=452709 ==========
cathiechen@tencent.com changed reviewers: + pdr@chromium.org, skobes@chromium.org
Description was changed from ========== Fix font-size shaking issue in some pages In some page content text is autosized based on multiplier of LayoutView. In order to make the multiplier of LayoutView more steady, use LayoutView itself as widthProvider instead of finding deepestBlockContainingAllText. BUG=452709 ========== to ========== Fix font-size shaking issue in some page In some page content is autosized based on multiplier of LayoutView. In order to make the multiplier of LayoutView more steady, use LayoutView itself as widthProvider instead of finding deepestBlockContainingAllText. BUG=452709 ==========
This change special-cases the LayoutView but isn't there still an issue with multipliers changing? Do you know the cause of the changing multiplier issue? Or is the issue specific to LayoutView? https://codereview.chromium.org/2817443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2817443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:907: if (root->isTable() || root->isTableCell() || root->isLayoutView()) Can you update the comment in TextAutosizer.h to mention that LayoutView is a special-case too?
On 2017/04/12 at 17:55:33, pdr. wrote: > This change special-cases the LayoutView but isn't there still an issue with multipliers changing? Do you know the cause of the changing multiplier issue? Or is the issue specific to LayoutView? > > https://codereview.chromium.org/2817443003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): > > https://codereview.chromium.org/2817443003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/TextAutosizer.cpp:907: if (root->isTable() || root->isTableCell() || root->isLayoutView()) > Can you update the comment in TextAutosizer.h to mention that LayoutView is a special-case too? Just read the description on the bug. Do you know why we don't re-compute the multipliers when the width changes?
On 2017/04/12 18:20:21, pdr. wrote: > On 2017/04/12 at 17:55:33, pdr. wrote: > > This change special-cases the LayoutView but isn't there still an issue with > multipliers changing? Do you know the cause of the changing multiplier issue? Or > is the issue specific to LayoutView? > > > > Just read the description on the bug. Do you know why we don't re-compute the > multipliers when the width changes? Sorry. I should paste the description here too. Yes, the change of LayoutView clusterMultiplier cause the font-size shrink in wikipedia. As we know, the clusterMultiplier of LayoutView cluster could be changed if we get different deepestBlockContainingAllText. In wikipedia case, content text is inside the LayoutView cluster. So the font-size of content text will be autosized by clusterMultiplier of LayoutView. The font-size won't change if the clusterMultiplier din't change. If LayoutView use itself as clusterWidthProvider, its clusterMultiplier won't change during layout passes. As to content text inside a block with variable specific width, in that case, content text will be re-layout anyway. It seems reasonable to get a font-size change.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
This patch needs a small rebase to apply on the bots. On 2017/04/13 at 14:54:05, cathiechen wrote: > As we know, the clusterMultiplier of LayoutView cluster could be changed if we get different deepestBlockContainingAllText. If this occurs, why don't we force the multipliers to be updated which will resize the links? I think this change makes sense but I'm wondering why the more general case (when deepestBlockContainingAllText changes) isn't already handled by existing code.
On 2017/04/14 01:44:06, pdr. wrote: > This patch needs a small rebase to apply on the bots. > > On 2017/04/13 at 14:54:05, cathiechen wrote: > > As we know, the clusterMultiplier of LayoutView cluster could be changed if we > get different deepestBlockContainingAllText. > > If this occurs, why don't we force the multipliers to be updated which will > resize the links? > Ah, I see:) This test case could reproduce the "tap link then font-size shrink" issue. BTW: it has attached on the bug. http://res.imtt.qq.com/qqbrowser_x5/cathiechen/TextAutosize/htmlWidthProvider... We have 3 layout passes here: 1. Finish loading page. The multiplier of LayoutView is (800-140)/320. At this time, <div id="panel"> is empty, so we found <div id="content"> as our deepestBlockContainingAllText. After layout, the cluster of LayoutView will be deleted. 2. Js insert text into <div id="panel">, so the deepestBlockContainingAllText become <body>, and the multiplier become 800/320. But we won't layout <div id="content"> this time. Because needsLayout of <div id="content"> is false, and this new cluster have no idea if its multiplier has been changed. So the result is that text in <div id="panel"> is bigger than <div id="content">. 3. Users tap the link in <div id="content">. It'll trigger layout of <div id="content">. After layout <div id="content">, the new multiplier(800/320) will be applied. So the font-size shrink. > I think this change makes sense but I'm wondering why the more general case > (when deepestBlockContainingAllText changes) isn't already handled by existing > code. Looks like this isn't a normal scenario in general case(block with specific width). However it happens to layoutView. We do have some pages like: content on the left showed earlier than others, advertise on the right, and web information on the bottom.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/14 at 04:45:57, cathiechen wrote: > On 2017/04/14 01:44:06, pdr. wrote: > > This patch needs a small rebase to apply on the bots. > > > > On 2017/04/13 at 14:54:05, cathiechen wrote: > > > As we know, the clusterMultiplier of LayoutView cluster could be changed if we > > get different deepestBlockContainingAllText. > > > > If this occurs, why don't we force the multipliers to be updated which will > > resize the links? > > > > Ah, I see:) This test case could reproduce the "tap link then font-size shrink" issue. BTW: it has attached on the bug. > http://res.imtt.qq.com/qqbrowser_x5/cathiechen/TextAutosize/htmlWidthProvider... > > We have 3 layout passes here: > 1. Finish loading page. The multiplier of LayoutView is (800-140)/320. At this time, <div id="panel"> is empty, so we found <div id="content"> as our deepestBlockContainingAllText. After layout, the cluster of LayoutView will be deleted. > 2. Js insert text into <div id="panel">, so the deepestBlockContainingAllText become <body>, and the multiplier become 800/320. But we won't layout <div id="content"> this time. Because needsLayout of <div id="content"> is false, and this new cluster have no idea if its multiplier has been changed. So the result is that text in <div id="panel"> is bigger than <div id="content">. > 3. Users tap the link in <div id="content">. It'll trigger layout of <div id="content">. After layout <div id="content">, the new multiplier(800/320) will be applied. So the font-size shrink. > > > I think this change makes sense but I'm wondering why the more general case > > (when deepestBlockContainingAllText changes) isn't already handled by existing > > code. > > Looks like this isn't a normal scenario in general case(block with specific width). However it happens to layoutView. We do have some pages like: content on the left showed earlier than others, advertise on the right, and web information on the bottom. Ok sounds good, thanks for the explanation. LGTM
LGTM Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cathiechen@tencent.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Failed these 3 cases... fast/text-autosizing/narrow-iframe.html (Done. Explained at code.) fast/text-autosizing/font-scale-factor-change.html (Done) fast/text-autosizing/layout-after-append.html (Need rebaseline, widthProvider is changed from <body> to <html> after this patch) https://codereview.chromium.org/2817443003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (left): https://codereview.chromium.org/2817443003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:916: Conside of the inherit issuses. Move this check to DeepestBlockContainingAllText https://codereview.chromium.org/2817443003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2817443003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1014: return root; The special exceptions(table, td, html) check should move to here. Test case: <html style="font-size: 16px"> <body style="margin: 0;"> <div style="width: 400px"> This text should be autosized to just 20px computed font size, i.e. scaled up by 1.25x, since although this block is in an 800px wide iframe, it is in a 400px block and 400 / 320 = 1.25. More text, long enough to autosize... </div> </body> </html> 1. <div> will inherit <html> multiplier, because their DeepestBlockContainingAllText are same.(The DeepestBlockContainingAllText of <html> is <div>). 2. <div> apply multiplier of <html>. Approach: Move the special exceptions(table, td, html) check from ClusterWidthProvider to here. So their DeepestBlockContainingAllText are different. https://codereview.chromium.org/2817443003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1158: return true; These special exceptions(table, td, html) couldn't be inherited. Test case: <html style="font-size: 16px"> <body style="width:800px;"> <div style="width: 400px"> This text should be autosized to just 20px computed font size, i.e. scaled up by 1.25x, since although this block is in an 800px wide iframe, it is in a 400px block and 400 / 320 = 1.25. More text, long enough to autosize... </div> </body> </html> 1. <div>'s parent cluster is <body>, and they have same DeepestBlockContainingAllText. <div> inherit <body>. 2. Width of <body> is 800px, same to <html>. So <body> inherit <html>. 3. Finally, <div> will apply the multiplier of <html>. Approach: <html> couldn't be inherited.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:230: return block->IsTable() || block->IsTableCell() || block->IsLayoutView(); I don't think it'll be correct to move the IsTable and IsTableCell changes too. Aren't we just fixing an issue at the root of the document? If not, can you add a test of the table changes? https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1157: if (needMakeSelfAsWidthProvider(parent_deepest_block_containing_all_text)) I think this change is just making child clusters of the layout view's cluster return true. That will basically special-case the children of the layout view's cluster to be a widerOrNarrowerDescendant. I think that'll fix the bug on wikipedia but fail to fix the more general bug. I think it's okay to special-case the root (layout view) but special-casing only direct descendants of the layout view seems like it's too local of a fix. Fix idea: in your previous response you said "Looks like this isn't a normal scenario in general case(block with specific width). However it happens to layoutView.". I wonder if we could remove layout view special cases so that the layout view works more like a normal block with a specific width? https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:810: content->setInnerHTML(content->innerHTML()); Sorry I missed this before. Why is this needed?
Sorry for the delay. I just come back from a trip:) https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:230: return block->IsTable() || block->IsTableCell() || block->IsLayoutView(); On 2017/04/20 06:25:46, pdr. wrote: > I don't think it'll be correct to move the IsTable and IsTableCell changes too. > Aren't we just fixing an issue at the root of the document? If not, can you add > a test of the table changes? yes, table and td have the same inherit issues. I could add test cases. Case 1: <table><tr><td style="width:800px"> <div style="width: 400px"> This text should be autosized to just 20px computed font size, i.e. scaled up by 1.25x, since although this block is in an 800px wide iframe, it is in a 400px block and 400 / 320 = 1.25. More text, long enough to autosize... </div> </td></tr></table> 1. <div> will inherit <td>'s multiplier. In IsWiderOrNarrowerDescendant(), we use DeepestBlockContainingAllText(not ClusterWidthProvider) of <td> to determine if <div> should inherit from <td>. 2. <td>'s multiplier is 800 / 320 = 2.5. So the multiplier of <td> isn't as expected. 3. Approach: Move needMakeSelfAsWidthProvider() from TextAutosizer::ClusterWidthProvider() to TextAutosizer::DeepestBlockContainingAllText(). After we move needMakeSelfAsWidthProvider to DeepestBlockContainingAllText. Case2 still failed. Case 2: <table><tr><td style="width:800px"> <div id="outer" style="width:800px;"> <div id="inner" style="width: 400px"> This text should be autosized to just 20px computed font size, i.e. scaled up by 1.25x, since although this block is in an 800px wide iframe, it is in a 400px block and 400 / 320 = 1.25. More text, long enough to autosize... </div> </div> </td></tr></table> 1. "inner" <div> inherit "outer" <div> which inherit <td> 2. "inner" <div>'s multiplier is still 2.5. 3. Approach: <td> could not be inherited https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1157: if (needMakeSelfAsWidthProvider(parent_deepest_block_containing_all_text)) On 2017/04/20 06:25:46, pdr. wrote: > I think this change is just making child clusters of the layout view's cluster > return true. That will basically special-case the children of the layout view's > cluster to be a widerOrNarrowerDescendant. I think that'll fix the bug on > wikipedia but fail to fix the more general bug. I think it's okay to > special-case the root (layout view) but special-casing only direct descendants > of the layout view seems like it's too local of a fix. > > Fix idea: in your previous response you said "Looks like this isn't a normal > scenario in general case(block with specific width). However it happens to > layoutView.". I wonder if we could remove layout view special cases so that the > layout view works more like a normal block with a specific width? If we choose to fix the general case, here is the problem we need to solve: The font-size of a subCluster inside a specific width cluster(parentCluster) shouldn't change when the DeepestBlockContainingAllText of parentCluster changed. Here is a specific example: <div id="parentCluster" style="width:800px"> <div id="subCluster" style="width:700px"> ... text is long enough to autosize ... </div> <div id="EmptyAtFirst"></div> </div> 1. First layout pass: Multiplier of subCluster is 700/320. SubCluster inherits multiplier from parentCluster. The widthProvider is subCluster. So multiplier of parentCluster is 700/320. 2. Insert text into EmptyAtFirst: The widthProvider of parentCluster is changed to parentCluster itself. Multiplier of parentCluster is 800/320. 3. Trigger layout of subCluster: SubCluster inherits multiplier from parentCluster. The new multiplier is 800/320. The old multiplier is 700/320. So the font-size is changed. SubCluster inherit mutable multiplier from parentCluster is the fundamental problem. In order to reduce font-size change of subCluster. We could make parentCluster using itself as widthProvider, like table and td. But DeepestBlockContainingAllText is a basic design, there are risks to apply this change to all clusters with specific width. Consider of the risk, this patch just make LayoutView using itself as widthProvider, and others remain the same as before. And it will cause two inherit issues as I mentioned before. (Table and td have the same issues too). Or we could try another way. Any suggestions? https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:810: content->setInnerHTML(content->innerHTML()); On 2017/04/20 06:25:46, pdr. wrote: > Sorry I missed this before. Why is this needed? This is to trigger layout of <div id='content'>. So that the new multiplier will be applied to <div id='content'>.
cathiechen@tencent.com changed reviewers: + jamesxwei@tencent.com
I'm back to this issue;) Sorry for the delay again... In order to avoid unnecessary confusion. Let's block this issue temporarily. I'll start a new review for inherit issue. https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2817443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1157: if (needMakeSelfAsWidthProvider(parent_deepest_block_containing_all_text)) On 2017/04/20 06:25:46, pdr (OOO Jury Duty). wrote: > > Fix idea: in your previous response you said "Looks like this isn't a normal > scenario in general case(block with specific width). However it happens to > layoutView.". I wonder if we could remove layout view special cases so that the > layout view works more like a normal block with a specific width? We had a misunderstanding here. Sorry for my bad English expressions... Actually, I mean "a normal block with a specific width" have font-size shaking issue too. LayoutView is treat as "a normal block with a specific width", so LayoutView has issus too. LayoutView have lots of these scenarios which could produce this issue: DeepestBlockContainingAllText of Layoutview grows wider as its foot section loaded. With this special treat of LayoutView, this could fix LayoutView scenarios. But the normal block case couldn't be fixed.
@skobes, I have jury duty until June 2nd. Could you please review this?
We have a public holiday in next few days. I'll be back on June 1st:) Ps: This patch should launch after Issue 2906033002. https://codereview.chromium.org/2906033002/ https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:3147: crbug.com/452709 fast/text-autosizing/layout-after-append.html [ NeedsRebaseline ] After this patch widthProvider of LayoutView will change from <body> to LayoutView itself. Effected by <body> margin the multiplier would change a bit (from (800-16)/320 to 800/320). NeedsRebaseline here.
It is not ideal to special-case the LayoutView - it sounds like a variation of this bug could still occur in non-LayoutView clusters, and that we don't always set needsLayout on every element whose multiplier would change if laid out. But I think solving the general problem would be very difficult, so I'm ok with this change if it helps on real websites. https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:3147: crbug.com/452709 fast/text-autosizing/layout-after-append.html [ NeedsRebaseline ] On 2017/05/27 04:06:52, cathiechentx wrote: > After this patch widthProvider of LayoutView will change from <body> to > LayoutView itself. Effected by <body> margin the multiplier would change a bit > (from (800-16)/320 to 800/320). > > NeedsRebaseline here. We have a new tool (webkit-patch rebaseline-cl) to fetch updated baselines. Can you give it a try? See instructions at: https://groups.google.com/a/chromium.org/d/msg/layout-dev/_t8JoYzeqh8/PHJUJ8O... https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1014: if (root && root->IsLayoutView()) I don't think we need to check if root is null here (if it were we would have crashed inside FindTextLeaf).
On 2017/05/31 01:50:26, skobes wrote: > It is not ideal to special-case the LayoutView - it sounds like a variation of > this bug could still occur in non-LayoutView clusters, and that we don't always > set needsLayout on every element whose multiplier would change if laid out. But > I think solving the general problem would be very difficult, so I'm ok with this > change if it helps on real websites. That's true. Couldn't find a proper way to solving the general problem. https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:3147: crbug.com/452709 fast/text-autosizing/layout-after-append.html [ NeedsRebaseline ] On 2017/05/31 01:50:26, skobes wrote: > We have a new tool (webkit-patch rebaseline-cl) to fetch updated baselines. Can > you give it a try? > > See instructions at: > https://groups.google.com/a/chromium.org/d/msg/layout-dev/_t8JoYzeqh8/PHJUJ8O... I couldn't start 'git cl try', need to be a committer... https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2817443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1014: if (root && root->IsLayoutView()) On 2017/05/31 01:50:26, skobes wrote: > I don't think we need to check if root is null here (if it were we would have > crashed inside FindTextLeaf). Done.
On 2017/06/01 15:36:27, cathiechentx wrote: > I couldn't start 'git cl try', need to be a committer... I ran rebaseline-cl on a copy of your patch in http://crrev.com/2920753004 and got new baselines for two tests: fast/text-autosizing/layout-after-append.html fast/text-autosizing/display-type-change.html But there were also some ref test failures: fast/text-autosizing/cluster-with-narrow-lca-and-cluster.html fast/text-autosizing/cluster-with-narrow-lca.html fast/text-autosizing/wide-in-narrow-overflow-scroll.html fast/text-autosizing/cluster-list-item.html fast/text-autosizing/wide-child.html Can you investigate why those are failing now?
On 2017/06/02 02:07:17, skobes wrote: > On 2017/06/01 15:36:27, cathiechentx wrote: > > I couldn't start 'git cl try', need to be a committer... > > I ran rebaseline-cl on a copy of your patch in http://crrev.com/2920753004 and > got new baselines for two tests: > > fast/text-autosizing/layout-after-append.html > fast/text-autosizing/display-type-change.html > > But there were also some ref test failures: > > fast/text-autosizing/cluster-with-narrow-lca-and-cluster.html > fast/text-autosizing/cluster-with-narrow-lca.html > fast/text-autosizing/wide-in-narrow-overflow-scroll.html > fast/text-autosizing/cluster-list-item.html > fast/text-autosizing/wide-child.html > > Can you investigate why those are failing now? Thanks very much! This patch should be applied after inherit issue fixed(https://codereview.chromium.org/2906033002/). Since it has been launched, all test cases should be success except one that need re-baseline: fast/text-autosizing/layout-after-append.html Would you please try it again?
On 2017/06/02 12:16:29, cathiechentx wrote: > Thanks very much! > > This patch should be applied after inherit issue > fixed(https://codereview.chromium.org/2906033002/). > > Since it has been launched, all test cases should be success except one that > need re-baseline: > > fast/text-autosizing/layout-after-append.html > > Would you please try it again? Yes the others are passing now. :) You can copy the new baseline images from the latest patchset on http://crrev.com/2920753004 and upload them here.
On 2017/06/02 23:59:30, skobes wrote: > > Yes the others are passing now. :) You can copy the new baseline images from > the latest patchset on http://crrev.com/2920753004 and upload them here. Done! :)
lgtm
The CQ bit was checked by cathiechen@tencent.com
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2817443003/#ps160001 (title: "rebase layout-after-append.html")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496716267822990,
"parent_rev": "3e863242981adb61aa416fda0607470df3ca0eb6", "commit_rev":
"47f9b0ff7d4594f6633956ac570966439b4d5125"}
Message was sent while issue was closed.
Description was changed from ========== Fix font-size shaking issue in some page In some page content is autosized based on multiplier of LayoutView. In order to make the multiplier of LayoutView more steady, use LayoutView itself as widthProvider instead of finding deepestBlockContainingAllText. BUG=452709 ========== to ========== Fix font-size shaking issue in some page In some page content is autosized based on multiplier of LayoutView. In order to make the multiplier of LayoutView more steady, use LayoutView itself as widthProvider instead of finding deepestBlockContainingAllText. BUG=452709 Review-Url: https://codereview.chromium.org/2817443003 Cr-Commit-Position: refs/heads/master@{#477198} Committed: https://chromium.googlesource.com/chromium/src/+/47f9b0ff7d4594f6633956ac5709... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/47f9b0ff7d4594f6633956ac5709... |
