Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339653003:
Enable the scroll of empty elements by setting the rect's size to 1. (Closed)
|
Created:
4 years, 3 months ago by sunyunjia Modified:
4 years, 3 months ago Reviewers:
bokan CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable the scroll of empty elements by setting the rect's size to 1.
LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is
empty after clipped by layerBounds. However, the rect to be scrolled might be
an empty rect originally. A common case is empty <a> elements used as named
anchors for navigation. This patch sets the size of the empty rect to 1 so that
it can still be scrolled.
BUG=646738
Committed: https://crrev.com/6d73ab5ed5d9b00bc48da255dc6599e90e72f599
Cr-Commit-Position: refs/heads/master@{#418856}
Patch Set 1 #Patch Set 2 : Rebase and format #
Total comments: 3
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by sunyunjia@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...
Description was changed from ========== Set the scrolling rect's size to 1 when empty, so that an empty element can also be scrolled. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ========== to ========== Set the scrolling rect's size to 1 when empty, so that an empty element can also be scrolled. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ==========
sunyunjia@chromium.org changed reviewers: + bokan@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Description was changed from ========== Set the scrolling rect's size to 1 when empty, so that an empty element can also be scrolled. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ========== to ========== Set the scrolling rect's size to 1 when empty, so that an empty element can also be scrolled. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ==========
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2339653003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scrolling/hashtag-autoscroll.html (right): https://codereview.chromium.org/2339653003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/hashtag-autoscroll.html:4: <div style="overflow-x:hidden"> Is this div needed?
https://codereview.chromium.org/2339653003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scrolling/hashtag-autoscroll.html (right): https://codereview.chromium.org/2339653003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/hashtag-autoscroll.html:4: <div style="overflow-x:hidden"> On 2016/09/14 21:43:15, bokan wrote: > Is this div needed? Yes. This div hasOverflowClip(), making it goto if (newRect.isEmpty()) return; And that's where the original solution fails. Without this "overflow-x:hidden", it will only be scrolled by document, and will be successful in the original solution.
lgtm https://codereview.chromium.org/2339653003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scrolling/hashtag-autoscroll.html (right): https://codereview.chromium.org/2339653003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/hashtag-autoscroll.html:4: <div style="overflow-x:hidden"> On 2016/09/14 23:58:39, sunyunjia wrote: > On 2016/09/14 21:43:15, bokan wrote: > > Is this div needed? > > Yes. This div hasOverflowClip(), making it goto > if (newRect.isEmpty()) > return; > And that's where the original solution fails. > > Without this "overflow-x:hidden", it will only be scrolled by document, and will > be successful in the original solution. Got it, thanks.
Please make sure your commit title is < 80 chars. Also, it might be nice to include in the description that a common case of empty rects is <a> elements used as named anchors for navigation.
Description was changed from ========== Set the scrolling rect's size to 1 when empty, so that an empty element can also be scrolled. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ========== to ========== Set the scrolling rect's size to 1 when empty, so that an empty element can also be scrolled. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation.This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ==========
Description was changed from ========== Set the scrolling rect's size to 1 when empty, so that an empty element can also be scrolled. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation.This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ========== to ========== Enable the scroll of empty elements by setting the rect's size to 1. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation.This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ==========
Description was changed from ========== Enable the scroll of empty elements by setting the rect's size to 1. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation.This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ========== to ========== Enable the scroll of empty elements by setting the rect's size to 1. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ==========
The CQ bit was checked by sunyunjia@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enable the scroll of empty elements by setting the rect's size to 1. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ========== to ========== Enable the scroll of empty elements by setting the rect's size to 1. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Enable the scroll of empty elements by setting the rect's size to 1. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 ========== to ========== Enable the scroll of empty elements by setting the rect's size to 1. LayoutBox::scrollRectToVisible will return if the new rect to be scrolled is empty after clipped by layerBounds. However, the rect to be scrolled might be an empty rect originally. A common case is empty <a> elements used as named anchors for navigation. This patch sets the size of the empty rect to 1 so that it can still be scrolled. BUG=646738 Committed: https://crrev.com/6d73ab5ed5d9b00bc48da255dc6599e90e72f599 Cr-Commit-Position: refs/heads/master@{#418856} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6d73ab5ed5d9b00bc48da255dc6599e90e72f599 Cr-Commit-Position: refs/heads/master@{#418856} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
