Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949373003:
Change Range.getBoundingClientRect() when the first rect is empty (Closed)
|
Created:
4 years, 7 months ago by kojii Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange Range.getBoundingClientRect() when the first rect is empty
crrev.com/376398 changed Range.getBoundingClientRect() to include the
first rectangle even when it is empty (either width or height is zero.)
This change improves when all rects are empty, but produces undesirable
result when the first rectangle is empty while the rest is not.
This patch changes it to return the first rectangle when all rectangles
are empty, following the spec change in [1].
content_editable_extractor_test.unitjs was reverted to before
crrev.com/376398.
[1] https://github.com/w3c/csswg-drafts/commit/0e7a5cbdea19397086e9423b508fe6f41decdcec
BUG=606662
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/27e0f0178d9c3d13bcd4dc96085cd0a749081d46
Cr-Commit-Position: refs/heads/master@{#393057}
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 2
Patch Set 3 : content_editable_extractor_test.unitjs #Patch Set 4 : Fixed test as per eae review #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== WIP: bounding BUG= ========== to ========== WIP: bounding BUG=606662 ==========
Description was changed from ========== WIP: bounding BUG=606662 ========== to ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty. Confirmed this is the behavior of Gecko, and sent a spec change to www-style[1]. [1] https://lists.w3.org/Archives/Public/www-style/2016May/0045.html BUG=606662 ==========
kojii@chromium.org changed reviewers: + eae@chromium.org
PTAL.
Let's wait a few days for the spec discussion to settle before landing this. https://codereview.chromium.org/1949373003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Range/get-bounding-client-rect-empty-and-non-empty.html (right): https://codereview.chromium.org/1949373003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Range/get-bounding-client-rect-empty-and-non-empty.html:28: Array.prototype.forEach.call(container.children, function (element) { Please don't do this. Data driven tests like this are very hard to debug and reason about. Instead please explicitly assert the individual conditions you want to test.
Oh, I forgot to mention; thank you for taking the time to look into this in detail and for reaching out to www-style with your findings.
Description was changed from ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty. Confirmed this is the behavior of Gecko, and sent a spec change to www-style[1]. [1] https://lists.w3.org/Archives/Public/www-style/2016May/0045.html BUG=606662 ========== to ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty. Confirmed this is the behavior of Gecko, and sent a spec change to www-style[1]. [1] https://lists.w3.org/Archives/Public/www-style/2016May/0045.html BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty. Confirmed this is the behavior of Gecko, and sent a spec change to www-style[1]. [1] https://lists.w3.org/Archives/Public/www-style/2016May/0045.html BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty. Confirmed this is the behavior of Gecko, and sent a spec change to www-style[1]. content_editable_extractor_test.unitjs was reverted to before crrev.com/376398. [1] https://lists.w3.org/Archives/Public/www-style/2016May/0045.html BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
sounds good, I added this to the F2F agenda. Also I'm not really sure reverting content_editable_extractor_test.unitjs is the right thing to do. Talk to you next week. https://codereview.chromium.org/1949373003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Range/get-bounding-client-rect-empty-and-non-empty.html (right): https://codereview.chromium.org/1949373003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Range/get-bounding-client-rect-empty-and-non-empty.html:28: Array.prototype.forEach.call(container.children, function (element) { On 2016/05/05 at 13:28:29, eae wrote: > Please don't do this. Data driven tests like this are very hard to debug and reason about. > > Instead please explicitly assert the individual conditions you want to test. Done, I'm not certain if I understand this correctly though, appreciate to tell me so if I misunderstand.
Description was changed from ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty. Confirmed this is the behavior of Gecko, and sent a spec change to www-style[1]. content_editable_extractor_test.unitjs was reverted to before crrev.com/376398. [1] https://lists.w3.org/Archives/Public/www-style/2016May/0045.html BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty, following the spec change in [1]. content_editable_extractor_test.unitjs was reverted to before crrev.com/376398. [1] https://github.com/w3c/csswg-drafts/commit/0e7a5cbdea19397086e9423b508fe6f41d... BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty, following the spec change in [1]. content_editable_extractor_test.unitjs was reverted to before crrev.com/376398. [1] https://github.com/w3c/csswg-drafts/commit/0e7a5cbdea19397086e9423b508fe6f41d... BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change improves when all rects are empty, but produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty, following the spec change in [1]. content_editable_extractor_test.unitjs was reverted to before crrev.com/376398. [1] https://github.com/w3c/csswg-drafts/commit/0e7a5cbdea19397086e9423b508fe6f41d... BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
WG is fine, the spec was updated. https://readable-email.org/list/www-style/topic/cssom-view-getboundingclientr... Is reverting content_editable_extractor_test.unitjs fine?
LGTM Yeah, go ahead and revert the change to the test.
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949373003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949373003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kojii@chromium.org changed reviewers: + dtseng@chromium.org
dtseng@, could you PTAL this file: chrome/browser/resources/chromeos/chromevox/common/content_editable_extractor_test.unitjs
lgtm
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949373003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949373003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change improves when all rects are empty, but produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty, following the spec change in [1]. content_editable_extractor_test.unitjs was reverted to before crrev.com/376398. [1] https://github.com/w3c/csswg-drafts/commit/0e7a5cbdea19397086e9423b508fe6f41d... BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Change Range.getBoundingClientRect() when the first rect is empty crrev.com/376398 changed Range.getBoundingClientRect() to include the first rectangle even when it is empty (either width or height is zero.) This change improves when all rects are empty, but produces undesirable result when the first rectangle is empty while the rest is not. This patch changes it to return the first rectangle when all rectangles are empty, following the spec change in [1]. content_editable_extractor_test.unitjs was reverted to before crrev.com/376398. [1] https://github.com/w3c/csswg-drafts/commit/0e7a5cbdea19397086e9423b508fe6f41d... BUG=606662 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/27e0f0178d9c3d13bcd4dc96085cd0a749081d46 Cr-Commit-Position: refs/heads/master@{#393057} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/27e0f0178d9c3d13bcd4dc96085cd0a749081d46 Cr-Commit-Position: refs/heads/master@{#393057} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
