Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1072403007:
Fixup a bug about SVG text selection. (Closed)
|
Created:
5 years, 8 months ago by hyunjunekim2 Modified:
5 years, 7 months ago CC:
blink-reviews, blink-reviews-rendering, zoltan1, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, kouhei+svg_chromium.org, Dominik Röttsches, ed+blinkwatch_opera.com, krit, f(malita), jchaffraix+rendering, gyuyoung2, Stephen Chennney, rwlbuis, pdr+svgwatchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFixup a bug about SVG text selection.
This CL is to fix bugs with collapsed whitespace for SVG text selection.
The expression is wrong for calculating distance.
BUG=477545
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194860
Patch Set 1 : Draft 1 #Patch Set 2 : add test #
Total comments: 7
Patch Set 3 : Draft 2 #Patch Set 4 : Draft 3 #Patch Set 5 : Draft 4 #Patch Set 6 : add a line. #
Total comments: 2
Patch Set 7 : Draft 5 #Patch Set 8 : Draft 6 #Patch Set 9 : Draft 7 #Patch Set 10 : Draft 8 #Patch Set 11 : Draft 9 #Patch Set 12 : Draft 10 #Patch Set 13 : Update test for no containing on the rect. #Patch Set 14 : Draft 11 #Patch Set 15 : Draft 12(Discard decimal point.) #Patch Set 16 : Draft 13 #
Total comments: 2
Patch Set 17 : Draft 14 #
Total comments: 3
Patch Set 18 : Draft 15 #Patch Set 19 : Draft 16 (remove floor) #
Total comments: 5
Patch Set 20 : Draft 17 #Patch Set 21 : Draft 18 #Patch Set 22 : Draft 19 #
Messages
Total messages: 79 (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.
On 2015/04/20 13:18:54, hyunjunekim2 wrote: > Hi, I'm hyunjunekim (mailto:hyunjune.kim@samsung.com, mailto:hykim0777@gamil.com). Could you > check this? Thank you. Please add test.
On 2015/04/20 13:24:28, fs wrote: > On 2015/04/20 13:18:54, hyunjunekim2 wrote: > > Hi, I'm hyunjunekim (mailto:hyunjune.kim@samsung.com, > mailto:hykim0777@gamil.com). Could you > > check this? Thank you. > > Please add test. Ok, Thank you. ;)
On 2015/04/20 13:24:53, hyunjunekim2 wrote: > On 2015/04/20 13:24:28, fs wrote: > > On 2015/04/20 13:18:54, hyunjunekim2 wrote: > > > Hi, I'm hyunjunekim (mailto:hyunjune.kim@samsung.com, > > mailto:hykim0777@gamil.com). Could you > > > check this? Thank you. > > > > Please add test. > > Ok, Thank you. ;) fs, Could you check test? Thank you.
https://codereview.chromium.org/1072403007/diff/20001/LayoutTests/svg/text/se... File LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html (right): https://codereview.chromium.org/1072403007/diff/20001/LayoutTests/svg/text/se... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:3: <head> Drop <html> and <head> https://codereview.chromium.org/1072403007/diff/20001/LayoutTests/svg/text/se... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:4: <title>Test for per-character selection with SVG font</title> This title doesn't look right. https://codereview.chromium.org/1072403007/diff/20001/LayoutTests/svg/text/se... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:8: <div> This <div> isn't needed. https://codereview.chromium.org/1072403007/diff/20001/LayoutTests/svg/text/se... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:9: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="200" height="200"> This could be just <svg>, and you'll get 300x150 dimensions, which seems sufficent. https://codereview.chromium.org/1072403007/diff/20001/LayoutTests/svg/text/se... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:23: var estCharWidth = textWidth / 18; This appears to expect a monospaced font, but I don't see one specified. https://codereview.chromium.org/1072403007/diff/20001/LayoutTests/svg/text/se... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:44: function pass(message) { I'd suggest using the js-test framework here instead of rolling something of your own. https://codereview.chromium.org/1072403007/diff/20001/Source/core/layout/svg/... File Source/core/layout/svg/LayoutSVGInlineText.cpp (right): https://codereview.chromium.org/1072403007/diff/20001/Source/core/layout/svg/... Source/core/layout/svg/LayoutSVGInlineText.cpp:187: float distance = powf(fragmentRect.x() + fragmentRect.width() / 2 - absolutePoint.x(), 2) + Looks like this could now be: float distance = (fragmentRect.center() - absolutePoint).diagonalLengthSquared();
fs, OMG... Remain bugs, when the blink read it. ======================================================================================================================================= <text id="text2" x="20" y="40"> <tspan id="tspan1" style="font-weight: bold;">happy</tspan> debugging !!</text> <text id="text3" x="20" y="80"> <tspan id="tspan2" style="font-size:30px;">happy</tspan> debugging !!</text> ======================================================================================================================================= So, Could you check LayoutSVGInlineText.cpp on |Patch Set 3 : Draft -2|? (If check like it, occur to fails for Layout-test.)
On 2015/04/21 15:45:44, hyunjunekim2 wrote: > fs, OMG... > Remain bugs, when the blink read it. > ======================================================================================================================================= > <text id="text2" x="20" y="40"> <tspan id="tspan1" style="font-weight: > bold;">happy</tspan> debugging !!</text> > <text id="text3" x="20" y="80"> <tspan id="tspan2" > style="font-size:30px;">happy</tspan> debugging !!</text> > ======================================================================================================================================= > So, Could you check LayoutSVGInlineText.cpp on |Patch Set 3 : Draft -2|? > (If check like it, occur to fails for Layout-test.) The change looks like an improvement - I suspect we don't want to drop the concept of "closest" though. I.e. there'll be some sort of priority order: 1. Fragments which contain the queried point 2. The fragment closest to the queried point (closest edge maybe, or some kind of center-line like previously)
On 2015/04/22 07:48:32, fs wrote: > On 2015/04/21 15:45:44, hyunjunekim2 wrote: > > fs, OMG... > > Remain bugs, when the blink read it. > > > ======================================================================================================================================= > > <text id="text2" x="20" y="40"> <tspan id="tspan1" style="font-weight: > > bold;">happy</tspan> debugging !!</text> > > <text id="text3" x="20" y="80"> <tspan id="tspan2" > > style="font-size:30px;">happy</tspan> debugging !!</text> > > > ======================================================================================================================================= > > So, Could you check LayoutSVGInlineText.cpp on |Patch Set 3 : Draft -2|? > > (If check like it, occur to fails for Layout-test.) > > The change looks like an improvement - I suspect we don't want to drop the > concept of "closest" though. I.e. there'll be some sort of priority order: > > 1. Fragments which contain the queried point > 2. The fragment closest to the queried point (closest edge maybe, or some kind > of center-line like previously) fs, Now I updated this patch. Could you review this patch again? I am sorry...
On 2015/04/22 09:55:08, hyunjunekim2 wrote: > On 2015/04/22 07:48:32, fs wrote: ... > > The change looks like an improvement - I suspect we don't want to drop the > > concept of "closest" though. I.e. there'll be some sort of priority order: > > > > 1. Fragments which contain the queried point > > 2. The fragment closest to the queried point (closest edge maybe, or some kind > > of center-line like previously) > > fs, Now I updated this patch. > Could you review this patch again? I am sorry... I actually think that there should be some kind of contains(point) test like in PS3 - distance calculations alone feels too fragile. Cases where fragments overlap to some degree would also need to be handled, but I think that just picking the last hit in that case should be ok. For the distance calculation, there'll be a lot of different ways to do it, clamping the queried point to the fragment rect and then computing distance might be one way.
On 2015/04/22 10:47:22, fs wrote: > On 2015/04/22 09:55:08, hyunjunekim2 wrote: > > On 2015/04/22 07:48:32, fs wrote: > ... > > > The change looks like an improvement - I suspect we don't want to drop the > > > concept of "closest" though. I.e. there'll be some sort of priority order: > > > > > > 1. Fragments which contain the queried point > > > 2. The fragment closest to the queried point (closest edge maybe, or some > kind > > > of center-line like previously) > > > > fs, Now I updated this patch. > > Could you review this patch again? I am sorry... > > I actually think that there should be some kind of contains(point) test like in > PS3 - distance calculations alone feels too fragile. Cases where fragments > overlap to some degree would also need to be handled, but I think that just > picking the last hit in that case should be ok. For the distance calculation, > there'll be a lot of different ways to do it, clamping the queried point to the > fragment rect and then computing distance might be one way. fs, I will arrange PS4 like in PS3. Currently this patch(PS4) passed other text selection tests in |LayoutTest/svg/text|. But i agree that feels too fragile. I am going to arrange expression and think smart way for PS4.
On 2015/04/23 02:18:45, hyunjunekim2 wrote: > On 2015/04/22 10:47:22, fs wrote: > > On 2015/04/22 09:55:08, hyunjunekim2 wrote: > > > On 2015/04/22 07:48:32, fs wrote: > > ... > > > > The change looks like an improvement - I suspect we don't want to drop the > > > > concept of "closest" though. I.e. there'll be some sort of priority order: > > > > > > > > 1. Fragments which contain the queried point > > > > 2. The fragment closest to the queried point (closest edge maybe, or some > > kind > > > > of center-line like previously) > > > > > > fs, Now I updated this patch. > > > Could you review this patch again? I am sorry... > > > > I actually think that there should be some kind of contains(point) test like > in > > PS3 - distance calculations alone feels too fragile. Cases where fragments > > overlap to some degree would also need to be handled, but I think that just > > picking the last hit in that case should be ok. For the distance calculation, > > there'll be a lot of different ways to do it, clamping the queried point to > the > > fragment rect and then computing distance might be one way. > > fs, > I will arrange PS4 like in PS3. Currently this patch(PS4) passed other text > selection tests in |LayoutTest/svg/text|. > But i agree that feels too fragile. I am going to arrange expression and think > smart way for PS4. fs, I arranged codes. If there is absolute-point between center and right edge of fragmentRect, selects rightEndge. In other cases selects left edge(like clamping the queried point to the fragment rect). And I ran tests in |LayoutTest/svg/text|. If there are fragile, Could you give me feedback?
On 2015/04/23 14:16:45, hyunjunekim2 wrote: ... > fs, I arranged codes. If there is absolute-point between center and right edge > of fragmentRect, > selects rightEndge. In other cases selects left edge(like clamping the queried > point to > the fragment rect). > And I ran tests in |LayoutTest/svg/text|. If there are fragile, Could you give > me feedback? I'd really prefer if exact matches are guaranteed to "win". https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... File Source/core/layout/svg/LayoutSVGInlineText.cpp (right): https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... Source/core/layout/svg/LayoutSVGInlineText.cpp:189: if (absolutePoint.x() > fragmentRect.center().x() && absolutePoint.x() < fragmentRect.maxX()) { Why not just pick the closest edge? Using the left edge when the point is outside to the right doesn't seem very good. https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... Source/core/layout/svg/LayoutSVGInlineText.cpp:190: selectedEdge = fragmentRect.rightEdgeMidPoint(); Rather than adding these new methods, you could do: FloatPoint selectedEdge = fragmentRect.center(); ... selectedEdge.move(-fragmentRect.width() / 2, 0); // left edge ... selectedEdge.move(fragmentRect.width() / 2, 0); // right edge
> https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... > Source/core/layout/svg/LayoutSVGInlineText.cpp:189: if (absolutePoint.x() > > fragmentRect.center().x() && absolutePoint.x() < fragmentRect.maxX()) { > Why not just pick the closest edge? Using the left edge when the point is > outside to the right doesn't seem very good. fs, Now replaced |absolutePoint.x() < fragmentRect.maxX()| with |fragmentRect.contains(absolutePoint)|. > Why not just pick the closest edge? We should consider cases if overlap, same distance. that is, if the selection case from the right edge is selected only if the right of the center and was enclosed in a rectangle. If same distance case, We need condition like |fragmentRect.contains(absolutePoint)|. Because if there isn't this condition in |for| statement, in front of the fragmentRect will be selected. And another case, If fragmentRect overlap(test case : svg/text/select-textLength-spacing-squeeze-4.svg), we consider this. |----|---|-----| | | x x | a | | o | | | x b x -----|---|-----| a : box b : box -x-x- : right edge o : center In this case, if there isn't this condition like |fragmentRect.contains(absolutePoint)|, shouldn't select |b| box. So that, we should select left edge of |a| box and right edge of |b| box. Could you give me feedback?
o : absolute-Point
a : SVGTextFragment b : SVGTextFragment
On 2015/04/24 13:40:15, hyunjunekim2 wrote: > > > https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... > > Source/core/layout/svg/LayoutSVGInlineText.cpp:189: if (absolutePoint.x() > > > fragmentRect.center().x() && absolutePoint.x() < fragmentRect.maxX()) { > > Why not just pick the closest edge? Using the left edge when the point is > > outside to the right doesn't seem very good. > > fs, Now replaced |absolutePoint.x() < fragmentRect.maxX()| with > |fragmentRect.contains(absolutePoint)|. > > > Why not just pick the closest edge? > > We should consider cases if overlap, same distance. that is, if the selection > case from the right edge > is selected only if the right of the center and was enclosed in a rectangle. > > If same distance case, We need condition like > |fragmentRect.contains(absolutePoint)|. Because if there isn't this condition > in |for| statement, in front of the fragmentRect will be selected. > > And another case, If fragmentRect overlap(test case : > svg/text/select-textLength-spacing-squeeze-4.svg), we consider this. > > |----|---|-----| > | | x x > | a | | o | > | | x b x > -----|---|-----| > a : box > b : box > -x-x- : right edge > o : center > > In this case, if there isn't this condition like > |fragmentRect.contains(absolutePoint)|, shouldn't select |b| box. > So that, we should select left edge of |a| box and right edge of |b| box. > > Could you give me feedback? If there's overlap, the fragment that comes last in visual (painting) order should be selected (I believe the spec. says "closest glyph box"). For LTR text, this means that fragment with higher indices are preferred, while for RTL it could be argued to be the opposite.
On 2015/04/24 14:00:28, fs wrote: > On 2015/04/24 13:40:15, hyunjunekim2 wrote: > > > > > > https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... > > > Source/core/layout/svg/LayoutSVGInlineText.cpp:189: if (absolutePoint.x() > > > > fragmentRect.center().x() && absolutePoint.x() < fragmentRect.maxX()) { > > > Why not just pick the closest edge? Using the left edge when the point is > > > outside to the right doesn't seem very good. > > > > fs, Now replaced |absolutePoint.x() < fragmentRect.maxX()| with > > |fragmentRect.contains(absolutePoint)|. > > > > > Why not just pick the closest edge? > > > > We should consider cases if overlap, same distance. that is, if the selection > > case from the right edge > > is selected only if the right of the center and was enclosed in a rectangle. > > > > If same distance case, We need condition like > > |fragmentRect.contains(absolutePoint)|. Because if there isn't this condition > > in |for| statement, in front of the fragmentRect will be selected. > > > > And another case, If fragmentRect overlap(test case : > > svg/text/select-textLength-spacing-squeeze-4.svg), we consider this. > > > > |----|---|-----| > > | | x x > > | a | | o | > > | | x b x > > -----|---|-----| > > a : box > > b : box > > -x-x- : right edge > > o : center > > > > In this case, if there isn't this condition like > > |fragmentRect.contains(absolutePoint)|, shouldn't select |b| box. > > So that, we should select left edge of |a| box and right edge of |b| box. > > > > Could you give me feedback? > > If there's overlap, the fragment that comes last in visual (painting) order > should be selected (I believe the spec. says "closest glyph box"). For LTR text, > this means that fragment with higher indices are preferred, while for RTL it > could be argued to be the opposite. fs, yes, this code select "closest glyph box". For LTR, RTL text, i checked. Could you give me idea?
On 2015/04/24 14:09:01, hyunjunekim2 wrote: > On 2015/04/24 14:00:28, fs wrote: > > On 2015/04/24 13:40:15, hyunjunekim2 wrote: > > > > > > > > > > https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... > > > > Source/core/layout/svg/LayoutSVGInlineText.cpp:189: if (absolutePoint.x() > > > > > > fragmentRect.center().x() && absolutePoint.x() < fragmentRect.maxX()) { > > > > Why not just pick the closest edge? Using the left edge when the point is > > > > outside to the right doesn't seem very good. > > > > > > fs, Now replaced |absolutePoint.x() < fragmentRect.maxX()| with > > > |fragmentRect.contains(absolutePoint)|. > > > > > > > Why not just pick the closest edge? > > > > > > We should consider cases if overlap, same distance. that is, if the > selection > > > case from the right edge > > > is selected only if the right of the center and was enclosed in a rectangle. > > > > > > If same distance case, We need condition like > > > |fragmentRect.contains(absolutePoint)|. Because if there isn't this > condition > > > in |for| statement, in front of the fragmentRect will be selected. > > > > > > And another case, If fragmentRect overlap(test case : > > > svg/text/select-textLength-spacing-squeeze-4.svg), we consider this. > > > > > > |----|---|-----| > > > | | x x > > > | a | | o | > > > | | x b x > > > -----|---|-----| > > > a : box > > > b : box > > > -x-x- : right edge > > > o : center > > > > > > In this case, if there isn't this condition like > > > |fragmentRect.contains(absolutePoint)|, shouldn't select |b| box. > > > So that, we should select left edge of |a| box and right edge of |b| box. > > > > > > Could you give me feedback? > > > > If there's overlap, the fragment that comes last in visual (painting) order > > should be selected (I believe the spec. says "closest glyph box"). For LTR > text, > > this means that fragment with higher indices are preferred, while for RTL it > > could be argued to be the opposite. > fs, > yes, this code select "closest glyph box". For LTR, RTL text, i checked. > Could you give me idea? sorry, I didn't check RTL. but this code select "closest glyph box"
On 2015/04/24 14:10:45, hyunjunekim2 wrote: > On 2015/04/24 14:09:01, hyunjunekim2 wrote: > > On 2015/04/24 14:00:28, fs wrote: > > > On 2015/04/24 13:40:15, hyunjunekim2 wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... > > > > > Source/core/layout/svg/LayoutSVGInlineText.cpp:189: if > (absolutePoint.x() > > > > > > > > fragmentRect.center().x() && absolutePoint.x() < fragmentRect.maxX()) { > > > > > Why not just pick the closest edge? Using the left edge when the point > is > > > > > outside to the right doesn't seem very good. > > > > > > > > fs, Now replaced |absolutePoint.x() < fragmentRect.maxX()| with > > > > |fragmentRect.contains(absolutePoint)|. > > > > > > > > > Why not just pick the closest edge? > > > > > > > > We should consider cases if overlap, same distance. that is, if the > > selection > > > > case from the right edge > > > > is selected only if the right of the center and was enclosed in a > rectangle. > > > > > > > > If same distance case, We need condition like > > > > |fragmentRect.contains(absolutePoint)|. Because if there isn't this > > condition > > > > in |for| statement, in front of the fragmentRect will be selected. > > > > > > > > And another case, If fragmentRect overlap(test case : > > > > svg/text/select-textLength-spacing-squeeze-4.svg), we consider this. > > > > > > > > |----|---|-----| > > > > | | x x > > > > | a | | o | > > > > | | x b x > > > > -----|---|-----| > > > > a : box > > > > b : box > > > > -x-x- : right edge > > > > o : center > > > > > > > > In this case, if there isn't this condition like > > > > |fragmentRect.contains(absolutePoint)|, shouldn't select |b| box. > > > > So that, we should select left edge of |a| box and right edge of |b| box. > > > > > > > > Could you give me feedback? > > > > > > If there's overlap, the fragment that comes last in visual (painting) order > > > should be selected (I believe the spec. says "closest glyph box"). For LTR > > text, > > > this means that fragment with higher indices are preferred, while for RTL it > > > could be argued to be the opposite. > > fs, > > yes, this code select "closest glyph box". For LTR, RTL text, i checked. > > Could you give me idea? > > sorry, I didn't check RTL. but this code select "closest glyph box" Is RTL text like Arabic?
On 2015/04/24 14:10:45, hyunjunekim2 wrote: > On 2015/04/24 14:09:01, hyunjunekim2 wrote: > > On 2015/04/24 14:00:28, fs wrote: > > > On 2015/04/24 13:40:15, hyunjunekim2 wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... > > > > > Source/core/layout/svg/LayoutSVGInlineText.cpp:189: if > (absolutePoint.x() > > > > > > > > fragmentRect.center().x() && absolutePoint.x() < fragmentRect.maxX()) { > > > > > Why not just pick the closest edge? Using the left edge when the point > is > > > > > outside to the right doesn't seem very good. > > > > > > > > fs, Now replaced |absolutePoint.x() < fragmentRect.maxX()| with > > > > |fragmentRect.contains(absolutePoint)|. > > > > > > > > > Why not just pick the closest edge? > > > > > > > > We should consider cases if overlap, same distance. that is, if the > > selection > > > > case from the right edge > > > > is selected only if the right of the center and was enclosed in a > rectangle. > > > > > > > > If same distance case, We need condition like > > > > |fragmentRect.contains(absolutePoint)|. Because if there isn't this > > condition > > > > in |for| statement, in front of the fragmentRect will be selected. > > > > > > > > And another case, If fragmentRect overlap(test case : > > > > svg/text/select-textLength-spacing-squeeze-4.svg), we consider this. > > > > > > > > |----|---|-----| > > > > | | x x > > > > | a | | o | > > > > | | x b x > > > > -----|---|-----| > > > > a : box > > > > b : box > > > > -x-x- : right edge > > > > o : center > > > > > > > > In this case, if there isn't this condition like > > > > |fragmentRect.contains(absolutePoint)|, shouldn't select |b| box. > > > > So that, we should select left edge of |a| box and right edge of |b| box. > > > > > > > > Could you give me feedback? > > > > > > If there's overlap, the fragment that comes last in visual (painting) order > > > should be selected (I believe the spec. says "closest glyph box"). For LTR > > text, > > > this means that fragment with higher indices are preferred, while for RTL it > > > could be argued to be the opposite. > > fs, > > yes, this code select "closest glyph box". For LTR, RTL text, i checked. > > Could you give me idea? > > sorry, I didn't check RTL. but this code select "closest glyph box" Based on the description you just gave it sounds like it would select |a| and not |b| in the case described. |b| would be the closest in that case AFAICS. Anyway, I think you should restructure to check contains() first, and only if that is false start determining edges etc.
On 2015/04/24 14:12:31, hyunjunekim2 wrote: ... > Is RTL text like Arabic? Could be, Hebrew works as well (and is likely easier to work with for selection.)
On 2015/04/24 14:15:09, fs wrote: > On 2015/04/24 14:10:45, hyunjunekim2 wrote: > > On 2015/04/24 14:09:01, hyunjunekim2 wrote: > > > On 2015/04/24 14:00:28, fs wrote: > > > > On 2015/04/24 13:40:15, hyunjunekim2 wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1072403007/diff/100001/Source/core/layout/svg... > > > > > > Source/core/layout/svg/LayoutSVGInlineText.cpp:189: if > > (absolutePoint.x() > > > > > > > > > > fragmentRect.center().x() && absolutePoint.x() < fragmentRect.maxX()) > { > > > > > > Why not just pick the closest edge? Using the left edge when the point > > is > > > > > > outside to the right doesn't seem very good. > > > > > > > > > > fs, Now replaced |absolutePoint.x() < fragmentRect.maxX()| with > > > > > |fragmentRect.contains(absolutePoint)|. > > > > > > > > > > > Why not just pick the closest edge? > > > > > > > > > > We should consider cases if overlap, same distance. that is, if the > > > selection > > > > > case from the right edge > > > > > is selected only if the right of the center and was enclosed in a > > rectangle. > > > > > > > > > > If same distance case, We need condition like > > > > > |fragmentRect.contains(absolutePoint)|. Because if there isn't this > > > condition > > > > > in |for| statement, in front of the fragmentRect will be selected. > > > > > > > > > > And another case, If fragmentRect overlap(test case : > > > > > svg/text/select-textLength-spacing-squeeze-4.svg), we consider this. > > > > > > > > > > |----|---|-----| > > > > > | | x x > > > > > | a | | o | > > > > > | | x b x > > > > > -----|---|-----| > > > > > a : box > > > > > b : box > > > > > -x-x- : right edge > > > > > o : center > > > > > > > > > > In this case, if there isn't this condition like > > > > > |fragmentRect.contains(absolutePoint)|, shouldn't select |b| box. > > > > > So that, we should select left edge of |a| box and right edge of |b| > box. > > > > > > > > > > Could you give me feedback? > > > > > > > > If there's overlap, the fragment that comes last in visual (painting) > order > > > > should be selected (I believe the spec. says "closest glyph box"). For LTR > > > text, > > > > this means that fragment with higher indices are preferred, while for RTL > it > > > > could be argued to be the opposite. > > > fs, > > > yes, this code select "closest glyph box". For LTR, RTL text, i checked. > > > Could you give me idea? > > > > sorry, I didn't check RTL. but this code select "closest glyph box" > > Based on the description you just gave it sounds like it would select |a| and > not |b| in the case described. |b| would be the closest in that case AFAICS. > Anyway, I think you should restructure to check contains() first, and only if > that is false start determining edges etc. fs, Sorry. give me chance. |----|---|-----| | | x x | a | | o | | | x b x -----|---|-----| a : SVGTextFragment(box) b : SVGTextFragment(box) -x-x- : right edge o : absolute-Point this case would select |b|. because closest from absolute-position. (ref test case : svg/text/select-textLength-spacing-squeeze-4.svg) this code compare distance between left edge of |a| box and right of |b| box. because this code select "closest glyph box"(|b| box). >Anyway, I think you should restructure to check contains() first, and only if >that is false start determining edges etc. how to this if (fragmentRect.contains(absolutePoint)) { if (absolutePoint.x() > fragmentRect.center().x()) { // absolute-Point is the right of center. selectedEdge.move(fragmentRect.width() / 2, 0); // right edge } else { // absolute-Point is the left of center. electedEdge.move(-fragmentRect.width() / 2, 0); // left edge } } else { // outer selectedEdge.move(-fragmentRect.width() / 2, 0); // left edge }
fs, Could you check PS8 and give me feedback?
On 2015/04/24 14:31:03, hyunjunekim2 wrote:
...
> |----|---|-----|
> | | x x
> | a | | o |
> | | x b x
> -----|---|-----|
> a : SVGTextFragment(box)
> b : SVGTextFragment(box)
> -x-x- : right edge
> o : absolute-Point
>
> this case would select |b|. because closest from absolute-position.
Ok, that seems correct.
> (ref test case : svg/text/select-textLength-spacing-squeeze-4.svg)
> this code compare distance between left edge of |a| box and right of |b| box.
> because this code select "closest glyph box"(|b| box).
That seems like severe overkill, since contains() should suffice to resolve this
case.
> >Anyway, I think you should restructure to check contains() first, and only if
> >that is false start determining edges etc.
> how to this
> if (fragmentRect.contains(absolutePoint)) {
> if (absolutePoint.x() > fragmentRect.center().x()) { // absolute-Point is
> the right of center.
> selectedEdge.move(fragmentRect.width() / 2, 0); // right edge
> } else { // absolute-Point is the left of center.
> electedEdge.move(-fragmentRect.width() / 2, 0); // left edge
> }
> } else { // outer
> selectedEdge.move(-fragmentRect.width() / 2, 0); // left edge
> }
If contains() is true, it should be possible to just set distance=0 (the only
closer we can get is by a fragment later in visual order.)
If the point is not contained, then we could compute the distance to the closest
point on the fragment box (by clamping the point to the box extents) and compare
against the current closest.
On 2015/04/24 14:48:23, fs wrote:
> On 2015/04/24 14:31:03, hyunjunekim2 wrote:
> ...
> > |----|---|-----|
> > | | x x
> > | a | | o |
> > | | x b x
> > -----|---|-----|
> > a : SVGTextFragment(box)
> > b : SVGTextFragment(box)
> > -x-x- : right edge
> > o : absolute-Point
> >
> > this case would select |b|. because closest from absolute-position.
>
> Ok, that seems correct.
>
> > (ref test case : svg/text/select-textLength-spacing-squeeze-4.svg)
> > this code compare distance between left edge of |a| box and right of |b|
box.
> > because this code select "closest glyph box"(|b| box).
>
> That seems like severe overkill, since contains() should suffice to resolve
this
> case.
>
> > >Anyway, I think you should restructure to check contains() first, and only
if
> > >that is false start determining edges etc.
> > how to this
> > if (fragmentRect.contains(absolutePoint)) {
> > if (absolutePoint.x() > fragmentRect.center().x()) { // absolute-Point
is
> > the right of center.
> > selectedEdge.move(fragmentRect.width() / 2, 0); // right edge
> > } else { // absolute-Point is the left of center.
> > electedEdge.move(-fragmentRect.width() / 2, 0); // left edge
> > }
> > } else { // outer
> > selectedEdge.move(-fragmentRect.width() / 2, 0); // left edge
> > }
>
> If contains() is true, it should be possible to just set distance=0 (the only
> closer we can get is by a fragment later in visual order.)
> If the point is not contained, then we could compute the distance to the
closest
> point on the fragment box (by clamping the point to the box extents) and
compare
> against the current closest.
OMG ...
fs, I fixed it. Could you give me feedback? and i'm sorry I can not not care.
On 2015/04/24 15:35:53, hyunjunekim2 wrote: > fs, I fixed it. Could you give me feedback? and i'm sorry I can not not care. I will update patch.(PS 11)
I updated patch.(PS 11)
fs, I updated patch applied clamping the queried point to the
fragment rect.(PS 14). I have some problem.
After apply patch set14, occurred to
test(select-textLength-spacing-squeeze-2.svg).
So I printed FragmentRect. and I read this
one(http://www.w3.org/TR/SVG/text.html#TextSelection)
w3c spec said that "the end glyph for the text selection operation is the glyph
within the same ‘text’ element whose glyph cell is closest to the pointer"
According to the results below, The nearest fragment is the seventh('
',x=29.916016, y=-35.000000) from absolute-point.
The eighth('S', x=31.172852, y=-35.000000) wants to be in
"select-textLength-spacing-squeeze-2.svg".
Could you give me advice?
And about clamping the queried point to the fragment rect, I reffed to this
site(http://gamedev.stackexchange.com/questions/44483/how-do-i-calculate-distance-from-a-point-to-a-rectangle)
Thank you.
======================================================================================
[absolute-point x = 31.000000, y = 4.000000]
Fragment 0, x=10.000000, y=-35.000000, width=6.108398, height=11.200000,
characterOffset=0, length=1, characters='T'
Fragment 1, x=14.586914, y=-35.000000, width=5.561523, height=11.200000,
characterOffset=1, length=1, characters='e'
Fragment 2, x=18.626953, y=-35.000000, width=5.000000, height=11.200000,
characterOffset=2, length=1, characters='x'
Fragment 3, x=22.105469, y=-35.000000, width=2.778320, height=11.200000,
characterOffset=3, length=1, characters='t'
Fragment 4, x=23.362305, y=-35.000000, width=2.778320, height=11.200000,
characterOffset=4, length=1, characters=' '
Fragment 5, x=24.619141, y=-35.000000, width=2.778320, height=11.200000,
characterOffset=5, length=1, characters='t'
Fragment 6, x=25.875977, y=-35.000000, width=5.561523, height=11.200000,
characterOffset=6, length=1, characters='o'
Fragment 7, x=29.916016, y=-35.000000, width=2.778320, height=11.200000,
characterOffset=7, length=1, characters=' '
Fragment 8, x=31.172852, y=-35.000000, width=6.669922, height=11.200000,
characterOffset=8, length=1, characters='S'
Fragment 9, x=36.321289, y=-35.000000, width=5.561523, height=11.200000,
characterOffset=9, length=1, characters='q'
Fragment 10, x=40.361328, y=-35.000000, width=5.561523, height=11.200000,
characterOffset=10, length=1, characters='u'
Fragment 11, x=44.401367, y=-35.000000, width=5.561523, height=11.200000,
characterOffset=11, length=1, characters='e'
Fragment 12, x=48.441406, y=-35.000000, width=5.561523, height=11.200000,
characterOffset=12, length=1, characters='e'
Fragment 13, x=52.481445, y=-35.000000, width=5.000000, height=11.200000,
characterOffset=13, length=1, characters='z'
Fragment 14, x=55.959961, y=-35.000000, width=5.561523, height=11.200000,
characterOffset=14, length=1, characters='e'
======================================================================================
Occurred to test fail(select-textLength-spacing-squeeze-2.svg).
On 2015/04/25 12:23:45, hyunjunekim2 wrote: > Occurred to test fail(select-textLength-spacing-squeeze-2.svg). Is test called 'select-textLength-spacing-squeeze-2.svg' wrong? See the results of the print, The result was 7th. But test are 8th wants. Questionable creation is fragments.
fs, I updated patch set. Could you check this patch and give me feedback?
And about patch set 15, I ran LayoutTest on |svg/text|. The results are pass of all.
On 2015/04/25 12:32:23, hyunjunekim2 wrote: > On 2015/04/25 12:23:45, hyunjunekim2 wrote: > > Occurred to test fail(select-textLength-spacing-squeeze-2.svg). > > Is test called 'select-textLength-spacing-squeeze-2.svg' wrong? It's possible. Based on the data you presented, I'd say 'yes'. PS15 looks pretty ok to me. My comments PS16 still applies to that though.
On 2015/04/27 09:02:00, fs wrote: > On 2015/04/25 12:32:23, hyunjunekim2 wrote: > > On 2015/04/25 12:23:45, hyunjunekim2 wrote: > > > Occurred to test fail(select-textLength-spacing-squeeze-2.svg). > > > > Is test called 'select-textLength-spacing-squeeze-2.svg' wrong? > > It's possible. Based on the data you presented, I'd say 'yes'. > > PS15 looks pretty ok to me. My comments PS16 still applies to that though. PS16 passed test named |select-textLength-spacing-squeeze-2.svg|. Could you give me a idea or a advice what to do?
On 2015/04/27 10:06:36, hyunjunekim2 wrote: > On 2015/04/27 09:02:00, fs wrote: > > On 2015/04/25 12:32:23, hyunjunekim2 wrote: > > > On 2015/04/25 12:23:45, hyunjunekim2 wrote: > > > > Occurred to test fail(select-textLength-spacing-squeeze-2.svg). > > > > > > Is test called 'select-textLength-spacing-squeeze-2.svg' wrong? > > > > It's possible. Based on the data you presented, I'd say 'yes'. > > > > PS15 looks pretty ok to me. My comments PS16 still applies to that though. > > PS16 passed test named |select-textLength-spacing-squeeze-2.svg|. > Could you give me a idea or a advice what to do? Figure out what points the test uses to perform selection, and how it derives those points.
> what points the test uses to perform selection? this points are extract on select-textLength-spacing-squeeze-2.svg. > how it derives those points? After insert |printf| on LayoutSVGInlineText::positionForPoint function, manually click green block on select-textLength-spacing-squeeze-2.svg. Like this I derived those points. > In addition, I thought of a reason for failure. When calculate distance, I thought that has a problem like decimal point error. So inserted |floorf| on code like |distance = floorf(fragmentRect.distanceFromPoint(absolutePoint));|. for example, this world was defined by 1 dimension. A : 0 (absolute position) B : 1.0001 (Box B) C : 1.0002 (Box C) If calculate distance, each distances are 1.00001(A - B) and 1.0002(A - C). But display device and mouse point don't have decimal point. So I used |floorf|. And passed tests. fs, Could you give me feedback or advice?
On 2015/04/27 12:52:56, hyunjunekim2 wrote: > > what points the test uses to perform selection? > this points are extract on select-textLength-spacing-squeeze-2.svg. > > > how it derives those points? > After insert |printf| on LayoutSVGInlineText::positionForPoint function, > manually click green block on select-textLength-spacing-squeeze-2.svg. > Like this I derived those points. This is not how the _test_ derives the points it uses though. It does so using getStartPositionOfChar and getExtentsOfChar AFAICS. > > In addition, > I thought of a reason for failure. When calculate distance, I thought that > has a problem like decimal point error. So inserted |floorf| on code like > |distance = floorf(fragmentRect.distanceFromPoint(absolutePoint));|. > > for example, this world was defined by 1 dimension. > A : 0 (absolute position) > B : 1.0001 (Box B) > C : 1.0002 (Box C) > If calculate distance, each distances are 1.00001(A - B) and 1.0002(A - C). > But display device and mouse point don't have decimal point. So I used > |floorf|. > And passed tests. I don't think that's the right approach. While the point itself may not have any actual subpixel resolution (yet), fragments most certainly do. (for some reason I forgotten to submit the following comments...) https://codereview.chromium.org/1072403007/diff/290001/Source/platform/geomet... File Source/platform/geometry/FloatRect.cpp (right): https://codereview.chromium.org/1072403007/diff/290001/Source/platform/geomet... Source/platform/geometry/FloatRect.cpp:165: float FloatRect::distanceFromPoint(const FloatPoint& p) squaredDistance... This is probably a bit to specialized ATM, so we could as well have this as a static helper function in LayoutSVGInlineText.cpp. https://codereview.chromium.org/1072403007/diff/290001/Source/platform/geomet... Source/platform/geometry/FloatRect.cpp:167: float closestX, closestY; If you made this a FloatPoint, you wouldn't need to open-code as much (could use operator- and diagonalLengthSquared.)
fs, I updated PS 17. Except |floorf|, When click red area on select-textLength-spacing-squeeze-2.svg, i re-printed box size, absolute-positon and etc. The difference between 7th and 8th is about 0.03 for calculating distance. I confused. Could you tell me the approach? ========================================================================== [absolute-position x=31.000000,y=3.000000] [0][x=10.000000,y=-66.000000,width=6.108398,height=11.157269][distance=3567.541260] characters='T' [1][x=14.586914,y=-66.000000,width=5.561523,height=11.157269][distance=3463.537842] characters='e' [2][x=18.626953,y=-66.000000,width=5.000000,height=11.157269][distance=3400.143311] characters='x' [3][x=22.105469,y=-66.000000,width=2.778320,height=11.157269][distance=3383.189453] characters='t' [4][x=23.362305,y=-66.000000,width=2.778320,height=11.157269][distance=3369.395020] characters=' ' [5][x=24.619141,y=-66.000000,width=2.778320,height=11.157269][distance=3358.759766] characters='t' [6][x=25.875977,y=-66.000000,width=5.561523,height=11.157269][distance=3345.781494] characters='o' [7][x=29.916016,y=-66.000000,width=2.778320,height=11.157269][distance=3345.781494] characters=' ' [8][x=31.172852,y=-66.000000,width=6.669922,height=11.157269][distance=3345.811279] characters='S' [9][x=36.321289,y=-66.000000,width=5.561523,height=11.157269][distance=3374.097656] characters='q' [10][x=40.361328,y=-66.000000,width=5.561523,height=11.157269][distance=3433.416016] characters='u' [11][x=44.401367,y=-66.000000,width=5.561523,height=11.157269][distance=3525.378174] characters='e' [12][x=48.441406,y=-66.000000,width=5.561523,height=11.157269][distance=3649.984131] characters='e' [13][x=52.481445,y=-66.000000,width=5.000000,height=11.157269][distance=3807.233887] characters='z' [14][x=55.959961,y=-66.000000,width=5.561523,height=11.157269][distance=3968.781250] characters='e' ==========================================================================
On 2015/04/27 14:15:25, hyunjunekim2 wrote: > fs, I updated PS 17. > Except |floorf|, When click red area on select-textLength-spacing-squeeze-2.svg, > i re-printed box size, absolute-positon and etc. > The difference between 7th and 8th is about 0.03 for calculating distance. > I confused. Could you tell me the approach? > ========================================================================== > [absolute-position x=31.000000,y=3.000000] This is likely to be the culprit. What's interesting is what absStartPos (and absEndPos) ends up being computed to in the test, and how EventSender treats them (it truncates). So what likely happens is that the test framework computes a point that it then passes to eventSender.mouseMoveTo - which truncates it so that it no longer hits within the extents of the glyph that it intended to hit. Without this fix, that's not a problem, because the distance to the "right" glyph is still closer according to the old broken distance metric. So the test framework needs to be fixed to pick a point that is guaranteed to be within the extents of the glyph it intends to query. This could be as easy as: absStartPos.x = Math.ceil(absStartPos.x); absEndPos.x = Math.floor(absEndPos.x); It could also require more work. This fix should probably land separately.
https://codereview.chromium.org/1072403007/diff/310001/Source/core/layout/svg... File Source/core/layout/svg/LayoutSVGInlineText.cpp (right): https://codereview.chromium.org/1072403007/diff/310001/Source/core/layout/svg... Source/core/layout/svg/LayoutSVGInlineText.cpp:62: static float squaredDistance(const FloatRect& rect, const FloatPoint& point) squaredDistanceToClosestPoint[OnRect] https://codereview.chromium.org/1072403007/diff/310001/Source/core/layout/svg... Source/core/layout/svg/LayoutSVGInlineText.cpp:67: FloatPoint closestPoint(closestX, closestY); FloatPoint closestPoint; closestPoint.setX(...); closestPoint.setY(...); (or just set them directly in the constructor.) https://codereview.chromium.org/1072403007/diff/310001/Source/core/layout/svg... Source/core/layout/svg/LayoutSVGInlineText.cpp:198: // Discard decimal point. Nothing is discarded here.
On 2015/04/27 14:43:51, fs wrote: > On 2015/04/27 14:15:25, hyunjunekim2 wrote: > > fs, I updated PS 17. > > Except |floorf|, When click red area on > select-textLength-spacing-squeeze-2.svg, > > i re-printed box size, absolute-positon and etc. > > The difference between 7th and 8th is about 0.03 for calculating distance. > > I confused. Could you tell me the approach? > > ========================================================================== > > [absolute-position x=31.000000,y=3.000000] > > This is likely to be the culprit. What's interesting is what absStartPos (and > absEndPos) ends up being computed to in the test, and how EventSender treats > them (it truncates). So what likely happens is that the test framework computes > a point that it then passes to eventSender.mouseMoveTo - which truncates it so > that it no longer hits within the extents of the glyph that it intended to hit. > Without this fix, that's not a problem, because the distance to the "right" > glyph is still closer according to the old broken distance metric. So the test > framework needs to be fixed to pick a point that is guaranteed to be within the > extents of the glyph it intends to query. This could be as easy as: > > absStartPos.x = Math.ceil(absStartPos.x); > absEndPos.x = Math.floor(absEndPos.x); > > It could also require more work. This fix should probably land separately. fs, I ran select-textLength-spacing-squeeze-2.svg on the firefox(gecko). But when manually click red area, the text selection should work good. When make fragment, I thank that has a bug or problem. Or really like decimal point error. > about comments i updated patch set.
On 2015/04/27 15:44:01, hyunjunekim2 wrote: > On 2015/04/27 14:43:51, fs wrote: > > On 2015/04/27 14:15:25, hyunjunekim2 wrote: > > > fs, I updated PS 17. > > > Except |floorf|, When click red area on > > select-textLength-spacing-squeeze-2.svg, > > > i re-printed box size, absolute-positon and etc. > > > The difference between 7th and 8th is about 0.03 for calculating distance. > > > I confused. Could you tell me the approach? > > > ========================================================================== > > > [absolute-position x=31.000000,y=3.000000] > > > > This is likely to be the culprit. What's interesting is what absStartPos (and > > absEndPos) ends up being computed to in the test, and how EventSender treats > > them (it truncates). So what likely happens is that the test framework > computes > > a point that it then passes to eventSender.mouseMoveTo - which truncates it so > > that it no longer hits within the extents of the glyph that it intended to > hit. > > Without this fix, that's not a problem, because the distance to the "right" > > glyph is still closer according to the old broken distance metric. So the test > > framework needs to be fixed to pick a point that is guaranteed to be within > the > > extents of the glyph it intends to query. This could be as easy as: > > > > absStartPos.x = Math.ceil(absStartPos.x); > > absEndPos.x = Math.floor(absEndPos.x); > > > > It could also require more work. This fix should probably land separately. > > fs, > I ran select-textLength-spacing-squeeze-2.svg on the firefox(gecko). > But when manually click red area, the text selection should work good. Sure. Not really the "problem" we're here though, right? When you "manually click red area", you will trigger the contains(...) code-path (distance=0). Because of the overlap in this particular test, it's difficult to see the space between the 'S' and the 'o' (because those two overlap themselves.) > When make fragment, I thank that has a bug or problem. I don't understand what you mean by this. > Or really like decimal point error. I really don't like the floor(...) - it's applied on the squared metric, so it doesn't really make much sense IMO - if it happens to fix anything, that's by luck only. If there's a problem with how the test framework computes the points it uses, we should fix that.
I uploaded the suggest test framework fix: https://codereview.chromium.org/1108033002 It seems all svg/text tests still pass with that adjustment.
fs, I updated PS 19 was removed |floor|. And in https://codereview.chromium.org/1108033002, I requested. like |absStartPos.x = Math.ceil(absStartPos.x) + 4;|. Could you check this one? and give me feedback. Thank you.
>When you "manually click red area", you will trigger the contains(...)
code-path (distance=0).
We can not trigger distance=0. Because if you run
select-textLength-spacing-squeeze-2.svg,
Because of scale, It shouldn't be The case that contains(...) return true.
How to test
1) insert this code on |positionForPoint| function.
float distance = 0;
if (!fragmentRect.contains(absolutePoint)) {
distance = squaredDistanceToClosestPoint(fragmentRect, absolutePoint);
} else {
printf("distance == 0\n");
}
2) Manually run |select-textLength-spacing-squeeze-2.svg|
3) click red area or you want to area. maybe you couldn't see |distance == 0|.
>> When make fragment, I thank that has a bug or problem. > I don't understand what you mean by this. When make fragments, I think that has a bug which works wrong calculation for x, y width and height of fragments.
On 2015/04/28 02:46:13, hyunjunekim2 wrote: > >When you "manually click red area", you will trigger the contains(...) > code-path (distance=0). > > We can not trigger distance=0. Because if you run > select-textLength-spacing-squeeze-2.svg, > Because of scale, It shouldn't be The case that contains(...) return true. Ok, looks like the baseline is not adjusted correctly. Try changing it like this: - float baseline = m_scaledFont.fontMetrics().floatAscent(); + ASSERT(m_scalingFactor); + float baseline = m_scaledFont.fontMetrics().floatAscent() / m_scalingFactor; https://codereview.chromium.org/1072403007/diff/350001/LayoutTests/svg/text/s... File LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html (right): https://codereview.chromium.org/1072403007/diff/350001/LayoutTests/svg/text/s... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:11: <div id="passfail"> I don't see this <div> used for anything. https://codereview.chromium.org/1072403007/diff/350001/LayoutTests/svg/text/s... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:17: var a, b; What uses 'a' and 'b'? Remove? https://codereview.chromium.org/1072403007/diff/350001/LayoutTests/svg/text/s... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:27: var startPos = text1.getStartPositionOfChar(0); You could roll these chunks of code into a function similar to selectRange() in SelectionTestCase.js https://codereview.chromium.org/1072403007/diff/350001/LayoutTests/svg/text/s... LayoutTests/svg/text/select-svg-text-with-collapsed-whitespace.html:91: eventSender.mouseMoveTo(absStartPos.x, absStartPos.y - 15); Why does this (and the following) need to subtract a random number from the position? https://codereview.chromium.org/1072403007/diff/350001/Source/core/layout/svg... File Source/core/layout/svg/LayoutSVGInlineText.cpp (right): https://codereview.chromium.org/1072403007/diff/350001/Source/core/layout/svg... Source/core/layout/svg/LayoutSVGInlineText.cpp:198: } Drop the {}
On 2015/04/28 08:57:03, fs wrote: > On 2015/04/28 02:46:13, hyunjunekim2 wrote: > > >When you "manually click red area", you will trigger the contains(...) > > code-path (distance=0). > > > > We can not trigger distance=0. Because if you run > > select-textLength-spacing-squeeze-2.svg, > > Because of scale, It shouldn't be The case that contains(...) return true. > > Ok, looks like the baseline is not adjusted correctly. Try changing it like > this: > > - float baseline = m_scaledFont.fontMetrics().floatAscent(); > + ASSERT(m_scalingFactor); > + float baseline = m_scaledFont.fontMetrics().floatAscent() / > m_scalingFactor; fs, When apply this code, remain fail for test. the x position of fragment seems to be a problem. Does a attribute named |textLength="50"| affect to x position of fragment, When to calculate fragment?
On 2015/04/28 14:22:04, hyunjunekim2 wrote: > On 2015/04/28 08:57:03, fs wrote: > > On 2015/04/28 02:46:13, hyunjunekim2 wrote: > > > >When you "manually click red area", you will trigger the contains(...) > > > code-path (distance=0). > > > > > > We can not trigger distance=0. Because if you run > > > select-textLength-spacing-squeeze-2.svg, > > > Because of scale, It shouldn't be The case that contains(...) return true. > > > > Ok, looks like the baseline is not adjusted correctly. Try changing it like > > this: > > > > - float baseline = m_scaledFont.fontMetrics().floatAscent(); > > + ASSERT(m_scalingFactor); > > + float baseline = m_scaledFont.fontMetrics().floatAscent() / > > m_scalingFactor; > > fs, > When apply this code, remain fail for test. the x position of fragment seems to > be a problem. > Does a attribute named |textLength="50"| affect to x position of fragment, When > to calculate fragment? And the absolutePoint doesn't have decimal point.
On 2015/04/28 14:22:04, hyunjunekim2 wrote: > On 2015/04/28 08:57:03, fs wrote: > > On 2015/04/28 02:46:13, hyunjunekim2 wrote: > > > >When you "manually click red area", you will trigger the contains(...) > > > code-path (distance=0). > > > > > > We can not trigger distance=0. Because if you run > > > select-textLength-spacing-squeeze-2.svg, > > > Because of scale, It shouldn't be The case that contains(...) return true. > > > > Ok, looks like the baseline is not adjusted correctly. Try changing it like > > this: > > > > - float baseline = m_scaledFont.fontMetrics().floatAscent(); > > + ASSERT(m_scalingFactor); > > + float baseline = m_scaledFont.fontMetrics().floatAscent() / > > m_scalingFactor; > > fs, > When apply this code, remain fail for test. the x position of fragment seems to > be a problem. > Does a attribute named |textLength="50"| affect to x position of fragment, When > to calculate fragment? Is that maybe something that https://codereview.chromium.org/1108033002 would help with?
On 2015/04/28 14:33:34, fs wrote: > On 2015/04/28 14:22:04, hyunjunekim2 wrote: > > On 2015/04/28 08:57:03, fs wrote: > > > On 2015/04/28 02:46:13, hyunjunekim2 wrote: > > > > >When you "manually click red area", you will trigger the contains(...) > > > > code-path (distance=0). > > > > > > > > We can not trigger distance=0. Because if you run > > > > select-textLength-spacing-squeeze-2.svg, > > > > Because of scale, It shouldn't be The case that contains(...) return true. > > > > > > Ok, looks like the baseline is not adjusted correctly. Try changing it like > > > this: > > > > > > - float baseline = m_scaledFont.fontMetrics().floatAscent(); > > > + ASSERT(m_scalingFactor); > > > + float baseline = m_scaledFont.fontMetrics().floatAscent() / > > > m_scalingFactor; > > > > fs, > > When apply this code, remain fail for test. the x position of fragment seems > to > > be a problem. > > Does a attribute named |textLength="50"| affect to x position of fragment, > When > > to calculate fragment? > > Is that maybe something that https://codereview.chromium.org/1108033002 would > help with? Yes, I applied this patch. remained fail because the X coordinate error exists.
On 2015/04/28 14:37:18, hyunjunekim2 wrote: > On 2015/04/28 14:33:34, fs wrote: > > On 2015/04/28 14:22:04, hyunjunekim2 wrote: > > > On 2015/04/28 08:57:03, fs wrote: > > > > On 2015/04/28 02:46:13, hyunjunekim2 wrote: > > > > > >When you "manually click red area", you will trigger the contains(...) > > > > > code-path (distance=0). > > > > > > > > > > We can not trigger distance=0. Because if you run > > > > > select-textLength-spacing-squeeze-2.svg, > > > > > Because of scale, It shouldn't be The case that contains(...) return > true. > > > > > > > > Ok, looks like the baseline is not adjusted correctly. Try changing it > like > > > > this: > > > > > > > > - float baseline = m_scaledFont.fontMetrics().floatAscent(); > > > > + ASSERT(m_scalingFactor); > > > > + float baseline = m_scaledFont.fontMetrics().floatAscent() / > > > > m_scalingFactor; > > > > > > fs, > > > When apply this code, remain fail for test. the x position of fragment seems > > to > > > be a problem. > > > Does a attribute named |textLength="50"| affect to x position of fragment, > > When > > > to calculate fragment? > > > > Is that maybe something that https://codereview.chromium.org/1108033002 would > > help with? > > Yes, I applied this patch. remained fail because the X coordinate error exists. So, I am analyzing for the X coordinate.
On 2015/04/28 15:00:21, hyunjunekim2 wrote: ... > So, I am analyzing for the X coordinate. It's quite possible that the ceil() I tried to do should be performed on the user-space value rather than the "absolute" value, because viewBox scaling (5) will make it land on the wrong pixel anyway.
On 2015/04/28 15:27:08, fs wrote: > On 2015/04/28 15:00:21, hyunjunekim2 wrote: > ... > > So, I am analyzing for the X coordinate. > > It's quite possible that the ceil() I tried to do should be performed on the > user-space value rather than the "absolute" value, because viewBox scaling (5) > will make it land on the wrong pixel anyway. fs, What you say, Is picking position(absolute positon) wrong? Will we change to mouse position on user-space? right?
On 2015/04/28 15:37:33, hyunjunekim2 wrote: > On 2015/04/28 15:27:08, fs wrote: > > On 2015/04/28 15:00:21, hyunjunekim2 wrote: > > ... > > > So, I am analyzing for the X coordinate. > > > > It's quite possible that the ceil() I tried to do should be performed on the > > user-space value rather than the "absolute" value, because viewBox scaling (5) > > will make it land on the wrong pixel anyway. (I verified that doing this makes the two "squeeze" tests pass with the fix.) > What you say, Is picking position(absolute positon) wrong? I don't know which position you're referring to. If you refer to what I wrote above, then the position passed to the eventSender API definitely needs be "absolute". > Will we change to mouse position on user-space? right? No, the mouse position can't be user-space.
fs, Sorry. Could you explain user-space and absolute-space to me? Thank you.
On 2015/04/28 16:01:06, hyunjunekim2 wrote: > fs, > Sorry. Could you explain user-space and absolute-space to me? Thank you. Does user-space means state before multiply scale - matrix?
On 2015/04/28 16:03:03, hyunjunekim2 wrote: > On 2015/04/28 16:01:06, hyunjunekim2 wrote: > > fs, > > Sorry. Could you explain user-space and absolute-space to me? Thank you. > > Does user-space means state before multiply scale - matrix? User-space is the coordinate system in which an SVG shape is defined. For the <text> in question it position at <x=10,y=10> within its user-space. In this particular example you can get to the "absolute-space" by multiplying by 5 (because there's a viewBox="0 0 160 120" and the window/viewport size is 800x600). The test framework uses getScreenCTM() to get a matrix that will transform from the former into the latter. The values returned by getStartPositionOfChar is also in user-space (of the <text>).
On 2015/04/28 16:11:52, fs wrote: > On 2015/04/28 16:03:03, hyunjunekim2 wrote: > > On 2015/04/28 16:01:06, hyunjunekim2 wrote: > > > fs, > > > Sorry. Could you explain user-space and absolute-space to me? Thank you. > > > > Does user-space means state before multiply scale - matrix? > > User-space is the coordinate system in which an SVG shape is defined. For the > <text> in question it position at <x=10,y=10> within its user-space. In this > particular example you can get to the "absolute-space" by multiplying by 5 > (because there's a viewBox="0 0 160 120" and the window/viewport size is > 800x600). The test framework uses getScreenCTM() to get a matrix that will > transform from the former into the latter. The values returned by > getStartPositionOfChar is also in user-space (of the <text>). Do you recommend test framework changing absolute to user-space?
On 2015/04/28 16:29:31, hyunjunekim2 wrote: > On 2015/04/28 16:11:52, fs wrote: > > On 2015/04/28 16:03:03, hyunjunekim2 wrote: > > > On 2015/04/28 16:01:06, hyunjunekim2 wrote: > > > > fs, > > > > Sorry. Could you explain user-space and absolute-space to me? Thank you. > > > > > > Does user-space means state before multiply scale - matrix? > > > > User-space is the coordinate system in which an SVG shape is defined. For the > > <text> in question it position at <x=10,y=10> within its user-space. In this > > particular example you can get to the "absolute-space" by multiplying by 5 > > (because there's a viewBox="0 0 160 120" and the window/viewport size is > > 800x600). The test framework uses getScreenCTM() to get a matrix that will > > transform from the former into the latter. The values returned by > > getStartPositionOfChar is also in user-space (of the <text>). > > Do you recommend test framework changing absolute to user-space? If you mean adjust the endpoints in user-space (instead of absolute-space), then I think it could make sense.
fs, Hi, I updated PS 20. Could you check PS 20? I made adjustedPoint which has decimal point. How to make this. Using box width. Currently absolute position(mouse value) ignores decimal point info. So I made it. If box width 51.521484, we calculate 51.521484 / floor(51.521484) which is tolerance size. And I multiplied this tolerance on x and y of absolute point. This is adjustedPoint. Why to do this. Mouse value has decimal point error. Only Integer. So we need to adjust for mouse point. Thank you.
On 2015/04/29 04:48:42, hyunjunekim2 wrote: > fs, > Hi, I updated PS 20. Could you check PS 20? > I made adjustedPoint which has decimal point. > > How to make this. > Using box width. Currently absolute position(mouse value) ignores decimal point > info. > So I made it. If box width 51.521484, we calculate 51.521484 / floor(51.521484) > which is tolerance size. > And I multiplied this tolerance on x and y of absolute point. This is > adjustedPoint. > > Why to do this. > Mouse value has decimal point error. Only Integer. So we need to adjust for > mouse point. I don't think the code should deal with that (apart from that I think it looks fine). The issue is that the test framework is too optimistic when it comes to computing "mouse" positions. Maybe you should try a similar heuristic, but in the test framework instead (or just ceil() the point before as I suggested above.)
fs,
I think that the test frame work looks good to me.
Does position of fragments change to viewport-space(absolute)?
If need to multiply mousepoint and CTM(current transformation matrix) in
LayoutSVGInlineText::positionForPoint(C++).
How to get CTM matrix on LayoutSVGInlineText?
I confused.
> Do you recommend test framework changing absolute to user-space?
Do you say that need to multiply mousepoint and CTM for changing user-space?
Could you tell me what position(fragment or mouse position) go to where
space(absolute{viewport space} or user-spae)?
fs, M1 = SVGRoot.getScreenCTM M2 = element.getScreenCTM p = user space position(getStartPositionOfChar) Do you say to change like M1 * M2 * p?
On 2015/04/29 13:17:10, hyunjunekim2 wrote:
...
> I think that the test frame work looks good to me.
> Does position of fragments change to viewport-space(absolute)?
No, fragments (after their transform is applied) will be relative to the <text>.
> If need to multiply mousepoint and CTM(current transformation matrix) in
> LayoutSVGInlineText::positionForPoint(C++).
> How to get CTM matrix on LayoutSVGInlineText?
The position passed to positionForPoint(...) will have been inverse-transformed,
so that it is in the user-space of the <text>. Hence you don't need to get the
CTM.
> > Do you recommend test framework changing absolute to user-space?
> Do you say that need to multiply mousepoint and CTM for changing user-space?
>
> Could you tell me what position(fragment or mouse position) go to where
> space(absolute{viewport space} or user-spae)?
"mousepoint" = <screen CTM> * <query point in user-space as returned by
getStartPositionOfChar>
Fragment are essentially in user-space of <text>; mouse position is in "viewport
space", but will be modified when walking the tree for the actual hit-test (see
above).
The assumption that the "query point" maps to a pixel that in turn is still
contained within the (absolute) bounds of the glyph for which the query point
was produced is faulty, and needs to account for the CTM in some way. One way
could be:
@@ -63,8 +63,10 @@ function selectRange(id, start, end, expectedText) {
if (window.eventSender) {
// Trigger 'partial glyph selection' code, by adjusting the end x
position by half glyph width
var xOld = endPos.x;
endPos.x -= endExtent.width / 2 - 1;
+ var xOldStart = startPos.x;
+ startPos.x = Math.ceil(startPos.x);
var absStartPos = toAbsoluteCoordinates(startPos, element);
var absEndPos = toAbsoluteCoordinates(endPos, element);
(And removing the other ceil()/floor() I added, because that would not be useful
any more.)
Slightly more complicated would be to do the full round-trip (CTM^-1 *
(floor(CTM * p))) and compensate and re-transform the compensated result.
> M1 = SVGRoot.getScreenCTM
> M2 = element.getScreenCTM
> p = user space position(getStartPositionOfChar)
>
> Do you say to change like M1 * M2 * p?
No. (M2 contain M1, i.e. M2 = M1 * M, where M is the transform from the root to
the user-space of 'element')
fs, Ok, I will update new patch for test framework. And I have learned many SVG knowledge to you. Thank you for your help.
fs, I have a question. Why are the results different from gecko about svg positioning?
On 2015/04/30 13:38:48, hyunjunekim2 wrote: > fs, > I have a question. Why are the results different from gecko about svg > positioning? Differences in the font code most likely.
On 2015/04/30 13:41:49, fs wrote: > On 2015/04/30 13:38:48, hyunjunekim2 wrote: > > fs, > > I have a question. Why are the results different from gecko about svg > > positioning? > > Differences in the font code most likely. Thank you for your answer. :) Now i am going to arrange this patch.
fs, Could you check PS 22? Thank you.
lgtm
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/1072403007/410001
Message was sent while issue was closed.
Committed patchset #22 (id:410001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194860
Message was sent while issue was closed.
fs and pdr, I have some question and ask you something. If I get chromium.org and trybot authority, It will be better than now and more and more i will be bast on the blink. Is it possible? This is my activity. +https://codereview.chromium.org/search?closed=1&owner=hyunjune.kim@samsung.com +https://codereview.chromium.org/search?closed=1&owner=hykim0777@gmail.com I want to join in chromium team. :) Thank you.
Message was sent while issue was closed.
On 2015/05/04 at 12:47:16, hyunjune.kim wrote: > fs and pdr, > I have some question and ask you something. > If I get chromium.org and trybot authority, > It will be better than now and more and more i will be bast on the blink. > Is it possible? > > This is my activity. > +https://codereview.chromium.org/search?closed=1&owner=hyunjune.kim@samsung.com > +https://codereview.chromium.org/search?closed=1&owner=hykim0777@gmail.com > > I want to join in chromium team. :) > Thank you. I just nominated you for trybot and editbugs access. I think this typically takes a week to go through. A @chromium account requires becoming a committer.
Message was sent while issue was closed.
On 2015/05/04 21:43:02, pdr wrote: > On 2015/05/04 at 12:47:16, hyunjune.kim wrote: > > fs and pdr, > > I have some question and ask you something. > > If I get http://chromium.org and trybot authority, > > It will be better than now and more and more i will be bast on the blink. > > Is it possible? > > > > This is my activity. > > > +https://codereview.chromium.org/search?closed=1&owner=hyunjune.kim@samsung.com > > +https://codereview.chromium.org/search?closed=1&owner=hykim0777@gmail.com > > > > I want to join in chromium team. :) > > Thank you. > > I just nominated you for trybot and editbugs access. I think this typically > takes a week to go through. > > A @chromium account requires becoming a committer. pdf, Thank you. I will do my best more for becoming a committer. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
