Bug 199049 - Preview of <picture> element doesn't match element bounds
Summary: Preview of <picture> element doesn't match element bounds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
: 199048 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-06-19 18:19 PDT by Tim Horton
Modified: 2019-06-21 13:52 PDT (History)
7 users (show)

See Also:


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, Build Bot
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, Build Bot
no flags Details
Archive of layout-test-results from ews215 for win-future (13.44 MB, application/zip)
2019-06-19 19:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.99 MB, application/zip)
2019-06-19 20:11 PDT, Build Bot
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, Build Bot
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

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2019-06-19 18:19:18 PDT
Preview of <picture> element doesn't match element bounds
Comment 1 Tim Horton 2019-06-19 18:19:51 PDT
Created attachment 372509 [details]
Patch
Comment 2 Tim Horton 2019-06-19 18:19:53 PDT
<rdar://problem/51474402>
Comment 3 Tim Horton 2019-06-19 18:20:19 PDT
*** Bug 199048 has been marked as a duplicate of this bug. ***
Comment 4 Build Bot 2019-06-19 18:22:41 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-06-19 19:18:07 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-06-19 19:18:09 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-06-19 19:26:03 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2019-06-19 19:26:05 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2019-06-19 19:50:06 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-06-19 19:50:08 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2019-06-19 20:11:30 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2019-06-19 20:11:32 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2019-06-19 20:22:04 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2019-06-19 20:22:06 PDT Comment hidden (obsolete)
Comment 15 Simon Fraser (smfr) 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?
Comment 16 Tim Horton 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.
Comment 17 Tim Horton 2019-06-20 01:00:23 PDT
Created attachment 372545 [details]
Patch
Comment 18 Tim Horton 2019-06-20 13:09:07 PDT
smfr suggests looking at what intersection observer uses for its clipped-bounds-of-element computation
Comment 19 Tim Horton 2019-06-20 15:52:57 PDT
Created attachment 372597 [details]
Patch
Comment 20 Simon Fraser (smfr) 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
Comment 21 Tim Horton 2019-06-21 13:07:41 PDT
Created attachment 372642 [details]
Patch
Comment 22 WebKit Commit Bot 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>
Comment 23 WebKit Commit Bot 2019-06-21 13:52:53 PDT
All reviewed patches have been landed.  Closing bug.