Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1082723002:
Don't consider the viewBox in SVGSVGElement::selfHasRelativeLengths() (Closed)
|
Created:
5 years, 8 months ago by hyunjunekim2 Modified:
5 years, 8 months ago CC:
blink-reviews, krit, kouhei+svg_chromium.org, ed+blinkwatch_opera.com, f(malita), gyuyoung2, Stephen Chennney, pdr+svgwatchlist_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDon't consider the viewBox in SVGSVGElement::selfHasRelativeLengths()
This CL removes the check for a viewBox value in said method.
The viewBox will not be affected by changes to the viewport(s) of
its ancestor(s).
And will not use hasAttribute for SVG values that can animate.
BUG=235256
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193949
Patch Set 1 : Draft #Patch Set 2 : Add test. #Patch Set 3 : remove test #Patch Set 4 : animation test #Patch Set 5 : update test #Patch Set 6 : remove 'hasAttribute(SVGNames::viewBoxAttr)' on 'selfHasRelativeLengths()'. #Messages
Total messages: 20 (2 generated)
hyunjune.kim@samsung.com changed reviewers: + fs@opera.com, pdr@chromium.org, pfeldman@chromium.org
Hi, I'm hyunjunekim (hyunjune.kim@samsung.com, hykim0777@gamil.com). Could you check this? Thank you.
Could you add test for this?
On 2015/04/13 11:34:05, fs wrote: > Could you add test for this? fs, I will make test case.
fs, Could you check test? Give me feedback. I have referred to a test called "svg/custom/relative-sized-outer-svg.xhtml". Thank you.
On 2015/04/14 00:08:55, hyunjunekim2 wrote: > fs, Could you check test? Give me feedback. > I have referred to a test called "svg/custom/relative-sized-outer-svg.xhtml". > Thank you. I would expect the test to set a viewBox using an animation.
On 2015/04/14 08:29:03, fs wrote: > On 2015/04/14 00:08:55, hyunjunekim2 wrote: > > fs, Could you check test? Give me feedback. > > I have referred to a test called "svg/custom/relative-sized-outer-svg.xhtml". > > Thank you. > > I would expect the test to set a viewBox using an animation. fs, I will make test again.
On 2015/04/14 09:46:02, hyunjunekim2 wrote: > On 2015/04/14 08:29:03, fs wrote: > > On 2015/04/14 00:08:55, hyunjunekim2 wrote: > > > fs, Could you check test? Give me feedback. > > > I have referred to a test called > "svg/custom/relative-sized-outer-svg.xhtml". > > > Thank you. > > > > I would expect the test to set a viewBox using an animation. > > fs, I will make test again. fs, Could you give me feedback or idea to make animation test? I referred to a test called svgrect-animation-2.js. but It is some different. Thank you.
fs, Could you tell me if any part is wrong for a viewBox using an animation? Thank you.
On 2015/04/15 22:51:09, hyunjunekim2 wrote: > fs, Could you tell me if any part is wrong for a viewBox using an animation? > Thank you. fs, Update test. Please Could you check test. If has problem, give me feedback. Thank you.
On 2015/04/15 17:43:21, hyunjunekim2 wrote:
> On 2015/04/14 09:46:02, hyunjunekim2 wrote:
> > On 2015/04/14 08:29:03, fs wrote:
> > > On 2015/04/14 00:08:55, hyunjunekim2 wrote:
> > > > fs, Could you check test? Give me feedback.
> > > > I have referred to a test called
> > "svg/custom/relative-sized-outer-svg.xhtml".
> > > > Thank you.
> > >
> > > I would expect the test to set a viewBox using an animation.
> >
> > fs, I will make test again.
>
> fs, Could you give me feedback or idea to make animation test?
> I referred to a test called svgrect-animation-2.js. but It is some different.
> Thank you.
I think a start for a test might look something like:
<!DOCTYPE html>
<svg width="50%" viewBox="0 0 400 400">
<svg width="100" height="100">
<set attributeName="viewBox" to="0 0 1000 1000"
onbegin="mutateRootWidth()"/>
<rect width="1000" height="1000" fill="red"/>
</svg>
<rect width="100" height="100" fill="green"/>
</svg>
<script>
function mutateRootWidth() {
// Wait for a paint
testRunner.displayAsyncThen(function() {
document.querySelector('svg').setAttribute("width", "100%");
});
}
</script>
Untested, so probably needs tweaks and analysis so that it actually catches a
false -> true transition in that method. If you play around a bit with it you
might also find other bugs...
fs, I have studied viewBox effect for 3 days, I think that 'hasAttribute(SVGNames::viewBoxAttr)' don't need on 'selfHasRelativeLengths()'. Because Viewbox don't affect 'Length' on layout. Could you explanation me why exist 'hasAttribute(SVGNames::viewBoxAttr)' on 'selfHasRelativeLengths()'? And in case only one hasAttribute(SVGNames::viewBoxAttr) is true, this is <svg x="0" y="0" width="150" height="150" viewBox="0 0 1000 1000">. ( m_y->currentValue()->isRelative() is false, m_width->currentValue()->isRelative() is false, m_height->currentValue()->isRelative() is false and hasAttribute(SVGNames::viewBoxAttr) is true) thank you.
On 2015/04/17 07:22:54, hyunjunekim2 wrote: > fs, I have studied viewBox effect for 3 days, I think that > 'hasAttribute(SVGNames::viewBoxAttr)' don't need on 'selfHasRelativeLengths()'. > Because Viewbox don't affect 'Length' on layout. Could you explanation me why > exist 'hasAttribute(SVGNames::viewBoxAttr)' on 'selfHasRelativeLengths()'? > And in case only one hasAttribute(SVGNames::viewBoxAttr) is true, this is <svg > x="0" y="0" width="150" height="150" viewBox="0 0 1000 1000">. > ( m_y->currentValue()->isRelative() is false, > m_width->currentValue()->isRelative() is false, > m_height->currentValue()->isRelative() is false and > hasAttribute(SVGNames::viewBoxAttr) is true) Yes, I considered this as a likely outcome too, in which case the hasAttribute(...) call should just be removed instead with a suitable motivation.
On 2015/04/17 08:25:54, fs wrote: > On 2015/04/17 07:22:54, hyunjunekim2 wrote: > > fs, I have studied viewBox effect for 3 days, I think that > > 'hasAttribute(SVGNames::viewBoxAttr)' don't need on > 'selfHasRelativeLengths()'. > > Because Viewbox don't affect 'Length' on layout. Could you explanation me why > > exist 'hasAttribute(SVGNames::viewBoxAttr)' on 'selfHasRelativeLengths()'? > > And in case only one hasAttribute(SVGNames::viewBoxAttr) is true, this is <svg > > x="0" y="0" width="150" height="150" viewBox="0 0 1000 1000">. > > ( m_y->currentValue()->isRelative() is false, > > m_width->currentValue()->isRelative() is false, > > m_height->currentValue()->isRelative() is false and > > hasAttribute(SVGNames::viewBoxAttr) is true) > > Yes, I considered this as a likely outcome too, in which case the > hasAttribute(...) call should just be removed instead with a suitable > motivation. fs, I removed it. Could you give me feedback? Thank you.
On 2015/04/17 12:43:15, hyunjunekim2 wrote: > On 2015/04/17 08:25:54, fs wrote: > > On 2015/04/17 07:22:54, hyunjunekim2 wrote: > > > fs, I have studied viewBox effect for 3 days, I think that > > > 'hasAttribute(SVGNames::viewBoxAttr)' don't need on > > 'selfHasRelativeLengths()'. > > > Because Viewbox don't affect 'Length' on layout. Could you explanation me > why > > > exist 'hasAttribute(SVGNames::viewBoxAttr)' on 'selfHasRelativeLengths()'? > > > And in case only one hasAttribute(SVGNames::viewBoxAttr) is true, this is > <svg > > > x="0" y="0" width="150" height="150" viewBox="0 0 1000 1000">. > > > ( m_y->currentValue()->isRelative() is false, > > > m_width->currentValue()->isRelative() is false, > > > m_height->currentValue()->isRelative() is false and > > > hasAttribute(SVGNames::viewBoxAttr) is true) > > > > Yes, I considered this as a likely outcome too, in which case the > > hasAttribute(...) call should just be removed instead with a suitable > > motivation. > > fs, I removed it. Could you give me feedback? Thank you. The description could use some additional love, maybe more like: " Don't consider the viewBox in SVGSVGElement::selfHasRelativeLengths() This CL removes the check for a viewBox value in said method. The viewBox will not be affected by changes to the viewport(s) of its ancestor(s). " If you have more to add, then please do so.
On 2015/04/17 13:25:23, fs wrote: > On 2015/04/17 12:43:15, hyunjunekim2 wrote: > > On 2015/04/17 08:25:54, fs wrote: > > > On 2015/04/17 07:22:54, hyunjunekim2 wrote: > > > > fs, I have studied viewBox effect for 3 days, I think that > > > > 'hasAttribute(SVGNames::viewBoxAttr)' don't need on > > > 'selfHasRelativeLengths()'. > > > > Because Viewbox don't affect 'Length' on layout. Could you explanation me > > why > > > > exist 'hasAttribute(SVGNames::viewBoxAttr)' on 'selfHasRelativeLengths()'? > > > > And in case only one hasAttribute(SVGNames::viewBoxAttr) is true, this is > > <svg > > > > x="0" y="0" width="150" height="150" viewBox="0 0 1000 1000">. > > > > ( m_y->currentValue()->isRelative() is false, > > > > m_width->currentValue()->isRelative() is false, > > > > m_height->currentValue()->isRelative() is false and > > > > hasAttribute(SVGNames::viewBoxAttr) is true) > > > > > > Yes, I considered this as a likely outcome too, in which case the > > > hasAttribute(...) call should just be removed instead with a suitable > > > motivation. > > > > fs, I removed it. Could you give me feedback? Thank you. > > The description could use some additional love, maybe more like: > > " > Don't consider the viewBox in SVGSVGElement::selfHasRelativeLengths() > > This CL removes the check for a viewBox value in said method. > The viewBox will not be affected by changes to the viewport(s) of > its ancestor(s). > " > > If you have more to add, then please do so. fs, I changed the description. and Thank you for your review and help.
LGTM if tests still pass.
The CQ bit was checked by hyunjune.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1082723002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193949 |
