Bug 199049

Summary:

Preview of <picture> element doesn't match element bounds

Product:

WebKit

Reporter:

Tim Horton <thorton>

Component:

New Bugs

Assignee:

Tim Horton <thorton>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aestes, commit-queue, ews-watchlist, rniwa, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ews103 for mac-highsierra	none
Archive of layout-test-results from ews107 for mac-highsierra-wk2	none
Archive of layout-test-results from ews215 for win-future	none
Archive of layout-test-results from ews112 for mac-highsierra	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch	none

Tim Horton

Reported 2019-06-19 18:19:18 PDT

Preview of <picture> element doesn't match element bounds

Attachments
Patch (23.51 KB, patch) 2019-06-19 18:19 PDT, Tim Horton	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.18 MB, application/zip) 2019-06-19 19:18 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.90 MB, application/zip) 2019-06-19 19:26 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews215 for win-future (13.44 MB, application/zip) 2019-06-19 19:50 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews112 for mac-highsierra (2.99 MB, application/zip) 2019-06-19 20:11 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.62 MB, application/zip) 2019-06-19 20:22 PDT, EWS Watchlist	no flags	Details
Patch (22.56 KB, patch) 2019-06-20 01:00 PDT, Tim Horton	no flags	Details Formatted Diff Diff
Patch (22.74 KB, patch) 2019-06-20 15:52 PDT, Tim Horton	no flags	Details Formatted Diff Diff
Patch (23.04 KB, patch) 2019-06-21 13:07 PDT, Tim Horton	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Tim Horton

Comment 1 2019-06-19 18:19:51 PDT

Created attachment 372509 [details] Patch

Tim Horton

Comment 2 2019-06-19 18:19:53 PDT

<rdar://problem/51474402>

Tim Horton

Comment 3 2019-06-19 18:20:19 PDT

*** Bug 199048 has been marked as a duplicate of this bug. ***

EWS Watchlist

Comment 4 2019-06-19 18:22:41 PDT Comment hidden (obsolete)

Attachment 372509 [details] did not pass style-queue: ERROR: Source/WebCore/testing/Internals.h:844: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/testing/Internals.h:852: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/testing/Internals.h:852: The parameter name "options" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

EWS Watchlist

Comment 5 2019-06-19 19:18:07 PDT Comment hidden (obsolete)

Comment on attachment 372509 [details] Patch Attachment 372509 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12526147 New failing tests: fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

EWS Watchlist

Comment 6 2019-06-19 19:18:09 PDT Comment hidden (obsolete)

Created attachment 372515 [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

EWS Watchlist

Comment 7 2019-06-19 19:26:03 PDT Comment hidden (obsolete)

Comment on attachment 372509 [details] Patch Attachment 372509 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12526187 New failing tests: fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

EWS Watchlist

Comment 8 2019-06-19 19:26:05 PDT Comment hidden (obsolete)

Created attachment 372517 [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

EWS Watchlist

Comment 9 2019-06-19 19:50:06 PDT Comment hidden (obsolete)

Comment on attachment 372509 [details] Patch Attachment 372509 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12526335 New failing tests: fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

EWS Watchlist

Comment 10 2019-06-19 19:50:08 PDT Comment hidden (obsolete)

Created attachment 372520 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

EWS Watchlist

Comment 11 2019-06-19 20:11:30 PDT Comment hidden (obsolete)

Comment on attachment 372509 [details] Patch Attachment 372509 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12526342 New failing tests: fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

EWS Watchlist

Comment 12 2019-06-19 20:11:32 PDT Comment hidden (obsolete)

Created attachment 372521 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6

EWS Watchlist

Comment 13 2019-06-19 20:22:04 PDT Comment hidden (obsolete)

Comment on attachment 372509 [details] Patch Attachment 372509 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12526437 New failing tests: fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

EWS Watchlist

Comment 14 2019-06-19 20:22:06 PDT Comment hidden (obsolete)

Created attachment 372522 [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.14.5

Simon Fraser (smfr)

Comment 15 2019-06-19 22:53:45 PDT

Comment on attachment 372509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372509&action=review > Source/WebCore/dom/Range.cpp:1824 > + if (containingBlock.hasOverflowClip() && (style.overflowX() != Overflow::Visible || style.overflowY() != Overflow::Visible)) { Maybe just containingBlock.hasOverflowClip()? > Source/WebCore/dom/Range.cpp:1839 > + auto bounding = quad.boundingBox(); bounding is what springboks do > Source/WebCore/dom/Range.h:119 > + enum class RectangleGenerationBehavior : uint8_t { BoundingRectBehavior? > Source/WebCore/dom/Range.h:121 > + IncludeRectsForDeepElements = 1 << 1, I don't know what deep elements are. It looks like this is really more about getting the bounds of elements inside of a clipped container?

Tim Horton

Comment 16 2019-06-19 23:20:09 PDT

(In reply to Simon Fraser (smfr) from comment #15) > Comment on attachment 372509 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372509&action=review > > > Source/WebCore/dom/Range.cpp:1824 > > + if (containingBlock.hasOverflowClip() && (style.overflowX() != Overflow::Visible || style.overflowY() != Overflow::Visible)) { > > Maybe just containingBlock.hasOverflowClip()? ... really? I definitely did not get that from reading. > > Source/WebCore/dom/Range.cpp:1839 > > + auto bounding = quad.boundingBox(); > > bounding is what springboks do Is it. > > Source/WebCore/dom/Range.h:119 > > + enum class RectangleGenerationBehavior : uint8_t { > > BoundingRectBehavior? Sure. > > Source/WebCore/dom/Range.h:121 > > + IncludeRectsForDeepElements = 1 << 1, > > I don't know what deep elements are. It looks like this is really more about > getting the bounds of elements inside of a clipped container? Previously, we would only include the bounds of each immediate child of the Range. Now, we recurse deeper into the tree instead. Doing this separately requires that we apply clipping to not regress many very simple cases.

Tim Horton

Comment 17 2019-06-20 01:00:23 PDT

Created attachment 372545 [details] Patch

Tim Horton

Comment 18 2019-06-20 13:09:07 PDT

smfr suggests looking at what intersection observer uses for its clipped-bounds-of-element computation

Tim Horton

Comment 19 2019-06-20 15:52:57 PDT

Created attachment 372597 [details] Patch

Simon Fraser (smfr)

Comment 20 2019-06-20 16:11:35 PDT

Comment on attachment 372597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372597&action=review > Source/WebCore/dom/Range.cpp:1824 > + LayoutRect localBounds = renderer->borderBoundingBox(); auto > Source/WebCore/dom/Range.cpp:1825 > + Optional<LayoutRect> rootClippedBounds = renderer->computeVisibleRectInContainer(localBounds, &renderer->view(), {false, false, visibleRectOptions }); Weird spacing. Auto? > Source/WebCore/dom/Range.cpp:1826 > + No blank line > Source/WebCore/dom/Range.cpp:1829 > + FloatRect snappedBounds = snapRectToDevicePixels(*rootClippedBounds, node->document().deviceScaleFactor()); auto > Source/WebCore/dom/Range.h:121 > + IncludeRectsForDeepElements = 1 << 1, Would be better as UseVisibleBounds or something I think > Source/WebCore/testing/Internals.cpp:5103 > +Internals::TextIndicatorData::TextIndicatorData(WebCore::TextIndicatorData data) TextIndicatorData is big to pass by value. > Source/WebCore/testing/Internals.cpp:5108 > +Internals::TextIndicatorData::~TextIndicatorData() = default > Source/WebCore/testing/Internals.h:833 > + struct TextIndicatorData { Name duplication with WebCore::TextIndicatorData is pretty confusing. > Source/WebCore/testing/Internals.h:837 > + TextIndicatorData(WebCore::TextIndicatorData); halp

Tim Horton

Comment 21 2019-06-21 13:07:41 PDT

Created attachment 372642 [details] Patch

WebKit Commit Bot

Comment 22 2019-06-21 13:52:51 PDT

Comment on attachment 372642 [details] Patch Clearing flags on attachment: 372642 Committed r246695: <https://trac.webkit.org/changeset/246695>

WebKit Commit Bot

Comment 23 2019-06-21 13:52:53 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.