Bug 197739

<rdar://problem/50708394>

Created attachment 369850 [details] WIP Frederic Wang <fwang@igalia.com> has finished: - imported the WPT tests - implemented parsing - made it work with <video> - experimented with the SVG <image> tag I'm gonna work on it based Fred's patch.

Created attachment 369851 [details] WIP

It's not clear that there's really consensus around this feature yet: https://github.com/mozilla/standards-positions/issues/129

Comment on attachment 369851 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=369851&action=review > Source/WebCore/html/HTMLImageElement.idl:45 > + // github.com/ojanvafai/intrinsicsize-attribute/blob/master/README.md The spec has moved to WICG. > Source/WebCore/html/HTMLVideoElement.idl:36 > + // github.com/ojanvafai/intrinsicsize-attribute/blob/master/README.md ditto > Source/WebCore/html/parser/HTMLParserIdioms.h:176 > +// https://github.com/ojanvafai/intrinsicsize-attribute Ditto > Source/WebCore/page/RuntimeEnabledFeatures.h:360 > + This should be m_intrinsicSizeAttributeEnabled. > Source/WebCore/rendering/RenderImage.cpp:225 > + unnecessary whitespace change

(In reply to Simon Fraser (smfr) from comment #4) > It's not clear that there's really consensus around this feature yet: > https://github.com/mozilla/standards-positions/issues/129 Right, this discussion ends up two months ago though. My understanding was that recent agreement was to separate the CSS aspect-ratio discussion from the intrinsicsize attribute discussion and that we can at least start experimenting intrinsicsize in order to provide more feedback at the W3C ¿? (cc'ing people from Google/Mozilla who might know the latest status)

Comment on attachment 369851 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=369851&action=review > Source/WebCore/html/parser/HTMLParserIdioms.cpp:504 > +bool parseIntrinsicSizeAttribute(StringView input, IntSize& intrinsicSize) > +{ > + unsigned newWidth = 0, newHeight = 0; > + auto splitResult = input.split('x'); > + auto it = splitResult.begin(); > + if (it != splitResult.end()) { > + if (auto parsedWidth = parseHTMLNonNegativeInteger(*it)) { > + ++it; > + if (it != splitResult.end()) { > + if (auto parsedHeight = parseHTMLNonNegativeInteger(*it)) { > + ++it; > + if (it == splitResult.end()) { > + newWidth = parsedWidth.value(); > + newHeight = parsedHeight.value(); > + } > + } > + } > + } > + } > + IntSize newSize(newWidth, newHeight); > + if (intrinsicSize != newSize) { > + intrinsicSize = newSize; > + return true; > + } > + return false; > +} This function does more than it says. It parses the string and sets intrinsicSize to newSize if they are different and return true or returns false is they are the same. At the same time when it returns false, it does not indicate whether there was an error in parsing the input string or newSize is the same as intrinsicSize. I would suggest the following: Expected<IntSize, HTMLIntegerParsingError> parseIntrinsicSizeAttribute(StringView input) { auto splitResult = input.split('x'); if (splitResult.size() == 2) { auto newWidth = parseHTMLNonNegativeInteger(splitResult[0]); if (!newWidth) return newWidth.error(); auto newHeight = parseHTMLNonNegativeInteger(splitResult[1]); if (!newHeight) return newHeight.error; return { newWidth.value(), newHeight.value() }; } return makeUnexpected(HTMLIntegerParsingError::Other); } And the caller can be like this: auto newSize = parseIntrinsicSizeAttribute(value); if (newSize && m_overriddenIntrinsicSize != newSize.value()) { m_overriddenIntrinsicSize = newSize.value(); if (is<RenderImage>(renderer())) downcast<RenderImage>(*renderer()).intrinsicSizeChanged(); }

Created attachment 370842 [details] WIP

Hi Fred & Said, Thanks for the review and sorry for the late reply. :) PS: The latest patch fixed the code review issues.

(In reply to Said Abou-Hallawa from comment #7) > This function does more than it says. It parses the string and sets > intrinsicSize to newSize if they are different and return true or returns > false is they are the same. At the same time when it returns false, it does > not indicate whether there was an error in parsing the input string or > newSize is the same as intrinsicSize. I would suggest the following: > > Expected<IntSize, HTMLIntegerParsingError> > parseIntrinsicSizeAttribute(StringView input) > { > auto splitResult = input.split('x'); > if (splitResult.size() == 2) { It seems StringView::SplitResult don't provide size(). So we need to travel iterator one by one. > auto newWidth = parseHTMLNonNegativeInteger(splitResult[0]); > if (!newWidth) > return newWidth.error(); > auto newHeight = parseHTMLNonNegativeInteger(splitResult[1]); > if (!newHeight) > return newHeight.error; > return { newWidth.value(), newHeight.value() }; > } > return makeUnexpected(HTMLIntegerParsingError::Other); > } > > And the caller can be like this: > > auto newSize = parseIntrinsicSizeAttribute(value); > if (newSize && m_overriddenIntrinsicSize != newSize.value()) { > m_overriddenIntrinsicSize = newSize.value(); > if (is<RenderImage>(renderer())) > downcast<RenderImage>(*renderer()).intrinsicSizeChanged(); > } Done. Thanks:)

(In reply to Simon Fraser (smfr) from comment #4) > It's not clear that there's really consensus around this feature yet: > https://github.com/mozilla/standards-positions/issues/129 Hi Simon, Thanks for the reply :) Yes, it seems there are intrinsic size, aspect ratio and mapping aspect ratio in width/height. Intrinsic size somehow has some advantage to solving the content jumping problem. The delay of calculating <img> size might cause the content jumping problem which effects user experience a lot. Both web developers and browser try many ways to fix it. For example, scroll anchoring, scroll the content back to the position. Or web developers use a 1x1 image first, calculate the actual height in advance, then replace the actual image. Intrinsic size makes us could calculate the layout size of <img> like other elements. It provides an easy way to solving this issue and also is good for performance. Intrinsic size could get the info what image could provide. So it could fix more scenarios than aspect ratio or mapping aspect ratio in width/height. - For aspect ratio, we could calculate the size when <img> has auto width. - For mapping aspect ratio in width/height, we could not make it when <img> has percent width. Intrinsic size and aspect ratio could work together like natural width/height with aspect ratio. So in this aspect, I think intrinsic size is a good resolution.

Is this the same as https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/hbhKRuBzZ4o ?

Hi Simon, Not the same, but they are trying to solve the same problem, i.e., calculating the layout size without waiting for image/video loading. This approach adds a new attribute intrinsicSize to get the intrinsic size of image/video. https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/hbhKRuBzZ4o is a new approach. It uses the width and height attribute to calculate the aspect ratio. For more details, see https://github.com/WICG/intrinsicsize-attribute/issues/16 . Both approaches could get the layout size in advance. So maybe we could open new another bug for the aspect ratio approach?

(In reply to cathiechen from comment #13) > Hi Simon, > > Not the same, but they are trying to solve the same problem, i.e., > calculating the layout size without waiting for image/video loading. > This approach adds a new attribute intrinsicSize to get the intrinsic size > of image/video. > https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/ > hbhKRuBzZ4o is a new approach. It uses the width and height attribute to > calculate the aspect ratio. For more details, see > https://github.com/WICG/intrinsicsize-attribute/issues/16 . > Both approaches could get the layout size in advance. So maybe we could open > new another bug for the aspect ratio approach? Yes, let's do that, and implement it first. It seems like a pretty safe change without having to add any new CSS properties.

(In reply to Simon Fraser (smfr) from comment #14) > Yes, let's do that, and implement it first. It seems like a pretty safe > change without having to add any new CSS properties. Great! Done! https://bugs.webkit.org/show_bug.cgi?id=201641

I think this can be closed as WONTFIX given the current discussions at W3C: https://github.com/WICG/intrinsicsize-attribute/#note-this-proposal-is-no-longer-active-instead-efforts-are-focused-on-the-alternate-approach-in-issue-16

It was resolved to instead modify spec to reuse width/height for this feature. Should we open a new bug for that? Otherwise, I believe, the behavior is mostly identical to intrinsicSize.

(In reply to Dima Voytenko from comment #17) > It was resolved to instead modify spec to reuse width/height for this > feature. Should we open a new bug for that? Otherwise, I believe, the > behavior is mostly identical to intrinsicSize. Cathie had already opened bug 201641

(In reply to Dima Voytenko from comment #17) > It was resolved to instead modify spec to reuse width/height for this > feature. Should we open a new bug for that? Otherwise, I believe, the > behavior is mostly identical to intrinsicSize. Is there some spec text somewhere that describes the whole image sizing algorithm? I looked and failed to find some.

(In reply to Simon Fraser (smfr) from comment #19) > (In reply to Dima Voytenko from comment #17) > > It was resolved to instead modify spec to reuse width/height for this > > feature. Should we open a new bug for that? Otherwise, I believe, the > > behavior is mostly identical to intrinsicSize. > > Is there some spec text somewhere that describes the whole image sizing > algorithm? I looked and failed to find some. I'm not sure if the changed ever happened in a spec, but discussion was https://github.com/WICG/intrinsicsize-attribute/issues/16 It seems chromium devs found more issues though, see https://bugs.webkit.org/show_bug.cgi?id=201641#c4