Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1541083002:
Fix invalid selection produced when dragging mouse outside the SVG text element (Closed)
|
Created:
5 years ago by hyunjunekim2 Modified:
4 years, 10 months ago CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement positionForPoint() for LayoutSVGRoot.
Not working text-selection when dragging mouse outside the SVG text element.
Because return either createPositionWithAffinity(0) or
createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|.
Implement positionForPoint() for LayoutSVGRoot and implement a way to find
the LayoutSVGText closest to the point.
BUG=445329
Committed: https://crrev.com/22889144f57acb143ddf25576f9ae309e6d88b13
Cr-Commit-Position: refs/heads/master@{#376141}
Patch Set 1 : Just test #Patch Set 2 : Draft #Patch Set 3 : Test1 #Patch Set 4 : #Patch Set 5 : Draft2 #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 5
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 12
Patch Set 14 : #Patch Set 15 : Starting Point #Patch Set 16 : Starting Point2 #Patch Set 17 : WIP(Draft) - No #Patch Set 18 : #Patch Set 19 : Draft(Ref. LayoutBox) #Patch Set 20 : #
Total comments: 7
Patch Set 21 : v1 #Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #Patch Set 25 : #
Total comments: 13
Patch Set 26 : #Patch Set 27 : #Patch Set 28 : #Patch Set 29 : #Patch Set 30 : #Patch Set 31 : #Patch Set 32 : #
Total comments: 5
Patch Set 33 : #Patch Set 34 : #Patch Set 35 : #Patch Set 36 : #
Total comments: 4
Patch Set 37 : #Patch Set 38 : #Patch Set 39 : #
Total comments: 4
Patch Set 40 : #
Total comments: 7
Patch Set 41 : #Patch Set 42 : #
Total comments: 3
Patch Set 43 : #Patch Set 44 : #
Total comments: 1
Patch Set 45 : #Patch Set 46 : #
Total comments: 4
Patch Set 47 : #Patch Set 48 : #
Total comments: 1
Patch Set 49 : #Patch Set 50 : #
Total comments: 5
Patch Set 51 : #Patch Set 52 : #Patch Set 53 : #Patch Set 54 : #
Total comments: 1
Patch Set 55 : #Patch Set 56 : #Patch Set 57 : #Patch Set 58 : #
Total comments: 2
Patch Set 59 : #Patch Set 60 : #Patch Set 61 : #
Total comments: 28
Patch Set 62 : #Patch Set 63 : #
Total comments: 11
Patch Set 64 : #Patch Set 65 : #Messages
Total messages: 137 (46 generated)
Description was changed from ========== WIP BUG=445329 ========== to ========== WIP BUG=445329 ==========
hyunjune.kim@samsung.com changed reviewers: + fs@opera.com, pdr@chromium.org
pdr, fs Could you check this idea? and if you are possible, give me advice? I realized positionForPoint on the LayoutSVGRoot. this logic has found near text layout by mouse point. and after transform mouse point to local coordinate, request text-layout text selection.
Can you create a test case so it's easier to review?
On 2016/01/06 at 18:31:46, pdr wrote: > Can you create a test case so it's easier to review? Seconded. I guess me might need something like this (unfortunately), but I'd really like to hear where the current code "goes wrong" (somewhere in LayoutBox::positionForPoint?). Attempt to describe the problem (and the proposed solution) really is of an essence here. As for current CL it looks like it does not handle transforms correctly (something that may well be the case for whatever current code runs though - although in a different way), and I don't think using state (m_latestPositionWithAffinity) in this way is a good thing.
pdr, fs Ok, I will make a test. and detail description.
Description was changed from ========== WIP BUG=445329 ========== to ========== WIP TEST=selection-dragging-outside.svg BUG=445329 ==========
Code description. https://codereview.chromium.org/1541083002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1541083002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:301: closestDescendant = descendant; 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. https://codereview.chromium.org/1541083002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:307: return createPositionWithAffinity(0); 2. If no has LayoutSVGText, return 0. https://codereview.chromium.org/1541083002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:324: transform = m_localToBorderBoxTransform * transform; 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * ....; 4. M = (ViewBox Matrix) * M https://codereview.chromium.org/1541083002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:325: absolutePoint = transform.inverse().mapPoint(absolutePoint); 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). https://codereview.chromium.org/1541083002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:332: return closestDescendant->positionForPoint(LayoutPoint(absolutePoint.x(), absolutePoint.y())); 6. After calculate position For Point on LayoutSVGText, return value.
Description was changed from ========== WIP TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. TEST=selection-dragging-outside.svg BUG=445329 ==========
On 2016/01/11 at 09:13:17, hyunjune.kim wrote: > Code description. Yes, I've no problem understanding what the code does =) - it's the "why" I'm more interested in (and preferably in the actual "Description" field - i.e what ends up as the commit message.)
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. TEST=selection-dragging-outside.svg BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. So re-define positionForPoint on the SVG Root, and implement selection logic. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. After re-define positionForPoint on the SVG Root, and implement selection logic. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. After re-define positionForPoint on the SVG Root, and implement selection logic. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.svg BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. After re-define positionForPoint on the SVG Root, and implement selection logic. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.html BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. After re-define positionForPoint on the SVG Root, and implement selection logic. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.html BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.html BUG=445329 ==========
fs, Could you review this patch? and give me a advice. Thank you.
On 2016/01/12 at 09:38:49, hyunjune.kim wrote: > fs, Could you review this patch? and give me a advice. Thank you. I'll reiterate what I wrote previously: "...but I'd really like to hear where the current code "goes wrong" (somewhere in LayoutBox::positionForPoint?). Attempt to describe the problem (and the proposed solution) really is of an essence here." (^- This is what I want to read about in the description. A high-level summary of why the current code doesn't work and what this CL does to address that. As such the bulleted list isn't very interesting.) And the following seems to still apply: "As for current CL it looks like it does not handle transforms correctly (something that may well be the case for whatever current code runs though - although in a different way)..." (Make a test where there are multiple <text> elements which have containers - <g> elements - as ancestors with transforms on them. The transforms should at the very least include a translation component.)
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.html BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on lineDirectionPosition, if has node(), return either createPositionWithAffinity(0) or createPositionWithAffinity(1). After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.html BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on lineDirectionPosition, if has node(), return either createPositionWithAffinity(0) or createPositionWithAffinity(1). After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.html BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on lineDirectionPosition, if has node(), return either createPositionWithAffinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.html BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on lineDirectionPosition, if has node(), return either createPositionWithAffinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. Like this. 1. Find closest LayoutSVGText from mouse which is local coordinate of LayoutSVGRoot. 2. If no has LayoutSVGText, return 0. 3. Make a Matrix M. M is (Root's child Matrix) * (Root's's child Matrix) * (Root's's's child Matirx) * .... (It's the full matrix to leaf.) 4. M = (ViewBox Matrix) * M 5. LayoutSVGText's local coordinate = M^-1 * point(local coodinate of SVG Root). 6. After calculate position For Point on LayoutSVGText, return value. TEST=selection-dragging-outside.html BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on lineDirectionPosition, if has node(), return either createPositionWithAffinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. TEST=selection-dragging-outside.html BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on lineDirectionPosition, if has node(), return either createPositionWithAffinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. TEST=selection-dragging-outside.html BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on lineDirectionPosition, if has node(), return either createPositionWithAffinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. TEST=selection-dragging-outside.html BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on lineDirectionPosition, if has node(), return either createPositionWithAffinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. TEST=selection-dragging-outside.html BUG=445329 ========== to ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. TEST=selection-dragging-outside.html BUG=445329 ==========
Description was changed from ========== Not Working text-selection when dragging mouse outside the SVG text element. Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. TEST=selection-dragging-outside.html BUG=445329 ========== to ========== Not working text-selection when dragging mouse outside the SVG text element. Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. TEST=selection-dragging-outside.html BUG=445329 ==========
On 2016/01/12 10:16:58, fs wrote: > On 2016/01/12 at 09:38:49, hyunjune.kim wrote: > > fs, Could you review this patch? and give me a advice. Thank you. > > I'll reiterate what I wrote previously: > > "...but I'd really like to hear where the current code "goes wrong" (somewhere > in LayoutBox::positionForPoint?). Attempt to describe the problem (and the > proposed solution) really is of an essence here." > > (^- This is what I want to read about in the description. A high-level summary > of why the current code doesn't work and what this CL does to address that. As > such the bulleted list isn't very interesting.) > Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). I wrote the description. > And the following seems to still apply: > > "As for current CL it looks like it does not handle transforms correctly > (something that may well be the case for whatever current code runs though - > although in a different way)..." > > (Make a test where there are multiple <text> elements which have containers - > <g> elements - as ancestors with transforms on them. The transforms should at > the very least include a translation component.) I changed codes on LayoutSVGRoot::positionForPoint. fs, Could you give me a advice? Thank you.
https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/svg/custom/mouse-move-on-svg-root-expected.txt (left): https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/svg/custom/mouse-move-on-svg-root-expected.txt:13: caret: position 0 of child 0 {#text} of child 3 {text} of child 1 {svg} of body Looks like all of these *-expected.txt updates could just go in TestExpectations. https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:287: LayoutObject* closestDescendant = nullptr; Since this is always LayoutSVGText it could be that type instead. https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:291: transformToParent.makeIdentity(); The AffineTransform constructor does this. https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:291: transformToParent.makeIdentity(); The AffineTransform constructor does this. https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:292: for (LayoutObject* descendant = firstChild(); descendant; descendant = descendant->nextInPreOrder(this)) { This will walk the entire SVG fragment. I think we should at least attempt to do some high-level pruning by doing breadth-first (rather than depth-first like this). That would be possible to write in a data-recursive manner, but I suspect that just adding a helper to SVGLayoutSupport (similar to how for instance layoutChildren is done) and make it code-recursive will be easier to deal with. I think LayoutBox::positionForPoint could serve as inspiration for approx how that will look. (How/if overlapping <text>s should be handled can be dealt with later - looks like the current "solution" would pick the first one in pre-order.) https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:297: transform.makeIdentity(); Done by the constructor. https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:308: boundaries = transform.mapRect(boundaries); |transform| here does not include any transform from |descendant|, and hence this maps from the user space of the ancestor of |descendant| rather than the user space of |descendant| itself. This in turn mean that the comparisons below are incorrect (compares points in different coordinate spaces.) Also, this will compare against the transformed bounding box of the <text> which means the distance can be smaller than it is in reality. Consider for example a <text> that is rotated 45 degrees. It'd be preferable to either use a quad or transform the query-point to the <text>s user space. https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:309: closestPoint.setX(std::max(std::min(absolutePoint.x(), boundaries.maxX()), boundaries.x())); Could also use clampTo(...) here instead of max(min(..., ...), ...). https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:322: return createPositionWithAffinity(0); There are most likely cases where we should be calling LayoutReplaced::positionForPoint - when outside of/clipped by the bbox of the root etc. https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:330: transformToParent = transformToParent * closestDescendant->localToParentTransform() * transform; This reinforces the comment above. Also, location() shouldn't be needed here. https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:334: if (absolutePoint.x() < 0) Why?
https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:292: for (LayoutObject* descendant = firstChild(); descendant; descendant = descendant->nextInPreOrder(this)) { On 2016/01/12 at 17:40:17, fs wrote: > I think LayoutBox::positionForPoint could serve as inspiration for approx how that will look. (How/if overlapping <text>s should be handled can be dealt with later - looks like the current "solution" would pick the first one in pre-order.) +1, LayoutBox::positionForPoint is a good starting point. If we do this, can you add FloatRect::distanceTo(FloatPoint) instead of implementing another copy of http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle? Maybe even as a patch before this patch with a simple unit test?
> https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:292: for > (LayoutObject* descendant = firstChild(); descendant; descendant = > descendant->nextInPreOrder(this)) { > This will walk the entire SVG fragment. I think we should at least attempt to do > some high-level pruning by doing breadth-first (rather than depth-first like > this). That would be possible to write in a data-recursive manner, but I suspect > that just adding a helper to SVGLayoutSupport (similar to how for instance > layoutChildren is done) and make it code-recursive will be easier to deal with. If travelling by doing breadth-first, I guess that has a problem which can't maintain parent's matrix. If should do breath-first, when en-queue child's, we need to en-queue with parent's world matrix that is matrix state till root. > https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:334: if > (absolutePoint.x() < 0) > Why? When set point to LayoutInlineText::positionForPoint, can't recognize negative value.
On 2016/01/13 at 15:44:39, hyunjune.kim wrote: > > https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:292: for > > (LayoutObject* descendant = firstChild(); descendant; descendant = > > descendant->nextInPreOrder(this)) { > > This will walk the entire SVG fragment. I think we should at least attempt to do > > some high-level pruning by doing breadth-first (rather than depth-first like > > this). That would be possible to write in a data-recursive manner, but I suspect > > that just adding a helper to SVGLayoutSupport (similar to how for instance > > layoutChildren is done) and make it code-recursive will be easier to deal with. > > If travelling by doing breadth-first, I guess that has a problem which can't maintain > parent's matrix. If should do breath-first, when en-queue child's, we need to en-queue > with parent's world matrix that is matrix state till root. Maintaining the CTM (explicitly or implicitly) will be a problem no matter how you dice it I believe. Written in a code-recursive way tracking state would be straight-forward. Data-recursion will be a tad more difficult, but only because it will need to be explicit. > > https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:334: if > > (absolutePoint.x() < 0) > > Why? > > When set point to LayoutInlineText::positionForPoint, can't recognize negative value. So it would trivially reject?
> https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:334: if > > > (absolutePoint.x() < 0) > > > Why? > > > > When set point to LayoutInlineText::positionForPoint, can't recognize negative > value. > > So it would trivially reject? I checked it. On int offset = closestDistanceBox->offsetForPositionInFragment(*closestDistanceFragment, absolutePoint.x() - closestDistancePosition, true); where is on LayoutSVGInlineText::positionForPoint, absolutePoint.x() - closestDistancePosition is negative value if drag mouse left on the text box. So I guess that we need protection codes like `https://codereview.chromium.org/1588863002/` which is just test.
fs, Could you check starting point on PS15? I will implement on LayouBox like PS15.
On 2016/01/14 at 05:23:56, hyunjune.kim wrote: > > https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:334: if > > > > (absolutePoint.x() < 0) > > > > Why? > > > > > > When set point to LayoutInlineText::positionForPoint, can't recognize negative > > value. > > > > So it would trivially reject? > > I checked it. > On int offset = closestDistanceBox->offsetForPositionInFragment(*closestDistanceFragment, absolutePoint.x() - closestDistancePosition, true); where is on LayoutSVGInlineText::positionForPoint, > absolutePoint.x() - closestDistancePosition is negative value if drag mouse left on the text box. > So I guess that we need protection codes like `https://codereview.chromium.org/1588863002/` which is just test. That might be one way to deal with it (it seems we may have similar issue for text-on-a-path and other cases though.) > fs, Could you check starting point on PS15? > I will implement on LayouBox like PS15. This is not what I meant by my reference to the LayoutBox method. This just calls through LayoutBox to call a specific function in SVGLayoutSupport which is obviously unnecessary. What LayoutBox::positionForPoint does is implement a similar type search as we need (find first intersecting or closest box and descend into it), but presumably we can't use it for our purposes - hence we need something else. If you want/find it easier to do it as a depth-first search that is fine just go ahead and do so - you need to get the transform handling right though as indicated earlier. I'm also not very fond of doing an essentially exhaustive search of the entire fragment, which suggest some form of pruning strategy be employed during the search.
On 2016/01/14 10:31:02, fs wrote: > On 2016/01/14 at 05:23:56, hyunjune.kim wrote: > > > > https://codereview.chromium.org/1541083002/diff/240001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:334: if > > > > > (absolutePoint.x() < 0) > > > > > Why? > > > > > > > > When set point to LayoutInlineText::positionForPoint, can't recognize > negative > > > value. > > > > > > So it would trivially reject? > > > > I checked it. > > On int offset = > closestDistanceBox->offsetForPositionInFragment(*closestDistanceFragment, > absolutePoint.x() - closestDistancePosition, true); where is on > LayoutSVGInlineText::positionForPoint, > > absolutePoint.x() - closestDistancePosition is negative value if drag mouse > left on the text box. > > So I guess that we need protection codes like > `https://codereview.chromium.org/1588863002/` which is just test. > > That might be one way to deal with it (it seems we may have similar issue for > text-on-a-path and other cases though.) > > > fs, Could you check starting point on PS15? > > I will implement on LayouBox like PS15. > > This is not what I meant by my reference to the LayoutBox method. This just > calls through LayoutBox to call a specific function in SVGLayoutSupport which is > obviously unnecessary. What LayoutBox::positionForPoint does is implement a > similar type search as we need (find first intersecting or closest box and > descend into it), but presumably we can't use it for our purposes - hence we > need something else. If you want/find it easier to do it as a depth-first search > that is fine just go ahead and do so - you need to get the transform handling > right though as indicated earlier. I'm also not very fond of doing an > essentially exhaustive search of the entire fragment, which suggest some form of > pruning strategy be employed during the search. fs, Thank you for your help. I'll try again. Thank you.
fs,
One must find the nearest TextObject?
How you do overlap if the parent object?
Like this,
<svg xmlns="http://www.w3.org/2000/svg" width="800" height="800">
<g>
<text x="30" y="50">Lorem Ipsum Dolor Sit Amet</text>
<rect x="30" y="80" width="100" height="100" fill="green"/>
</g>
<g>
<text x="30" y="60">Lorem Ipsum Dolor Sit Amet</text>
</g>
</svg>
`LayoutBox::positionForPoint` select just closest child, and
Propagate only closest child selected.
> <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800"> > <g> > <text x="30" y="50">Lorem Ipsum Dolor Sit Amet</text> > <rect x="30" y="80" width="100" height="100" fill="green"/> > </g> > <g> > <text x="30" y="60">Lorem Ipsum Dolor Sit Amet</text> > </g> > </svg> this case, If there is mouse on FloatPoint(150, 150), isn't text selected?
On 2016/01/14 14:02:25, hyunjunekim2 wrote: > > <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800"> > > <g> > > <text x="30" y="50">Lorem Ipsum Dolor Sit Amet</text> > > <rect x="30" y="80" width="100" height="100" fill="green"/> > > </g> > > <g> > > <text x="30" y="60">Lorem Ipsum Dolor Sit Amet</text> > > </g> > > </svg> > > this case, If there is mouse on FloatPoint(150, 150), isn't text selected? or FloatPoint(300, 300), drag.
On 2016/01/14 at 13:56:06, hyunjune.kim wrote: > fs, > One must find the nearest TextObject? > How you do overlap if the parent object? > Like this, > <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800"> > <g> > <text x="30" y="50">Lorem Ipsum Dolor Sit Amet</text> > <rect x="30" y="80" width="100" height="100" fill="green"/> > </g> > <g> > <text x="30" y="60">Lorem Ipsum Dolor Sit Amet</text> > </g> > </svg> > > `LayoutBox::positionForPoint` select just closest child, and > Propagate only closest child selected. If there's overlap, the "top-most" in the rendering order is probably preferable - suggesting a search starting at the last child and progressing through preceding siblings. This would also be consistent with hit-testing.
On 2016/01/14 14:24:36, fs wrote:
> On 2016/01/14 at 13:56:06, hyunjune.kim wrote:
> > fs,
> > One must find the nearest TextObject?
> > How you do overlap if the parent object?
> > Like this,
> > <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800">
> > <g>
> > <text x="30" y="50">Lorem Ipsum Dolor Sit Amet</text>
> > <rect x="30" y="80" width="100" height="100" fill="green"/>
> > </g>
> > <g>
> > <text x="30" y="60">Lorem Ipsum Dolor Sit Amet</text>
> > </g>
> > </svg>
> >
> > `LayoutBox::positionForPoint` select just closest child, and
> > Propagate only closest child selected.
>
> If there's overlap, the "top-most" in the rendering order is probably
preferable
> - suggesting a search starting at the last child and progressing through
> preceding siblings. This would also be consistent with hit-testing.
If Sub-Tree("top-most") that has no texts overlap sub-tree that has texts,
What happens in this case?
On 2016/01/14 at 14:45:29, hyunjune.kim wrote:
> On 2016/01/14 14:24:36, fs wrote:
> > On 2016/01/14 at 13:56:06, hyunjune.kim wrote:
> > > fs,
> > > One must find the nearest TextObject?
> > > How you do overlap if the parent object?
> > > Like this,
> > > <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800">
> > > <g>
> > > <text x="30" y="50">Lorem Ipsum Dolor Sit Amet</text>
> > > <rect x="30" y="80" width="100" height="100" fill="green"/>
> > > </g>
> > > <g>
> > > <text x="30" y="60">Lorem Ipsum Dolor Sit Amet</text>
> > > </g>
> > > </svg>
> > >
> > > `LayoutBox::positionForPoint` select just closest child, and
> > > Propagate only closest child selected.
> >
> > If there's overlap, the "top-most" in the rendering order is probably
preferable
> > - suggesting a search starting at the last child and progressing through
> > preceding siblings. This would also be consistent with hit-testing.
>
> If Sub-Tree("top-most") that has no texts overlap sub-tree that has texts,
> What happens in this case?
I guess you'd prefer the subtree with text before the one without.
fs, Could you check PS19? And How do you think this concept? I referred to `LayoutBox::positionForPoint`. Give me your advice. Thank you.
On 2016/01/15 07:31:41, hyunjunekim2 wrote: > fs, Could you check PS19? > And How do you think this concept? I referred to `LayoutBox::positionForPoint`. > Give me your advice. Thank you. fs, And i am going to write code's description on the patch.
https://codereview.chromium.org/1541083002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:485: if (child->style()->visibility() != VISIBLE) This is not correct at this level - 'visibility' can still be 'visible' for any descendant. https://codereview.chromium.org/1541083002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:488: if (!child->isSVGContainer() && !child->isSVGText()) "Hidden containers" still return true for isSVGContainer(), so this needs to be "(isSVGContainer() && !isSVGHiddenContainer())". https://codereview.chromium.org/1541083002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:492: float distance = bound.squaredDistanceTo(child->localToParentTransform().inverse().mapPoint(point)); Since you're using the inverse transform you should first check that it is invertable (if it isn't then the subtree can be skipped.) https://codereview.chromium.org/1541083002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:498: transform = localToParentTransform * closestLayoutObject->localToParentTransform(); No need to transform by |localToParentTransform| since |closestLayoutObject| and |child| can be brought to the same coordinate space just by applying their respective localToParentTransform()s (and thus so can their oBBs.) https://codereview.chromium.org/1541083002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:502: if (closestLayoutObject && bound.intersects(transform.mapRect(closestLayoutObject->objectBoundingBox()))) { Not quite sure why you'd need to check overlap between the two bboxes? If this new LayoutObject is closer, it seems we'd want to search it first? If both contain the point, I guess we'd want to search them both (in the order in which they were initially encountered)? This needs documentation/comments. https://codereview.chromium.org/1541083002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:513: if (closestLayoutObject && !closestLayoutObject->isSVGText()) { Use "early-out style" when possible - i.e here you'd do: if (!closestLayoutObject || closestLayoutObject->isSVGText()) return closestLayoutObject; ... https://codereview.chromium.org/1541083002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:519: localToParentTransform = localToParentTransform * closestLayoutObject->localToParentTransform().inverse(); Maintaining the transform like this looks really error-prone... It'd probably be better to just leave that out entirely - just inverse-transform the point and work completely in the local space, and then compute the CTM for the object found in LayoutSVGRoot.
fs, Could you check PS22? Thank you.
fs, Could you check PS25? and give me your advice. Thank you. ;)
You really need to have more tests for this... and decent enough documentation describing what the code is intended to do. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:296: while (layoutObject) { The computation done by this loop does not look correct (the transform of closestDescendant isn't included.) https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:304: if (toLayoutSVGText(closestDescendant)->location().x()) I don't see why this is needed. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:312: if (absolutePoint.x() < 0) Suggest to push this down into the callee chain. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:317: return closestDescendant->positionForPoint(LayoutPoint(absolutePoint.x(), absolutePoint.y())); LayoutPoint(absolutePoint) should work just fine. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:479: LayoutObject* SVGLayoutSupport::findSelectedLayoutSVGText(LayoutObject* layoutObject, const FloatPoint point) "Selected" doesn't seem like the correct term here. Maybe "nearest" or "closest" would be better. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:487: if (child->isSVGContainer() && child->isSVGHiddenContainer()) No need to check isSVGContainer() again. It would really be preferable to have this in the same check as the above. If you find this tricky to get right/rad, just add a helper function. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:490: FloatRect bound = child->objectBoundingBox(); Move down to just before you need it and rename to "boundingBox" or something a little more verbose. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:499: // If child is overlapped by closesLayoutObject, It's candiate to search sub-tree. Lot's of typos that needs fixing here ("closesLayoutObject"; "It's"; "candiate", etc) I also think that it might be better to have a comment like this either at the top of the function - describing the what it's intended to do - maybe at a higher level - and then shorter ones describing each step. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:502: if (closestLayoutObject && bound.contains(closestLayoutObject->objectBoundingBox())) { I don't quite see why the "overlapped" property matters - the distance is what will matter for the "nearest" property. Unless you have a different strategy (in which case you should document the "why".) Also consider a case where a <text> is found in this stage and a container is considered to be "closer" by the metric. Maybe it would be reasonable to visit "distance == 0" LayoutObjects directly and only "queue" LayoutObjects with "distance > 0" of something? https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:518: LayoutObject* result = findSelectedLayoutSVGText(closestLayoutObject, absolutePoint); if (LayoutObject* result = ...) return result; and then you can de-indent the following for-loop. It might also make sense to just let |closestLayoutObject| be the first in the vector and handle it in the loop with all the other candidates. https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h (right): https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h:111: static LayoutObject* findSelectedLayoutSVGText(LayoutObject*, const FloatPoint); const FloatPoint&
https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:304: if (toLayoutSVGText(closestDescendant)->location().x()) On 2016/01/18 12:44:11, fs wrote: > I don't see why this is needed. It needs codes. Because the transform is not applied to location of LayoutSVGText. So if no this codes, should disturb selection.
https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1541083002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:304: if (toLayoutSVGText(closestDescendant)->location().x()) On 2016/01/18 at 13:32:48, hyunjunekim2 wrote: > On 2016/01/18 12:44:11, fs wrote: > > I don't see why this is needed. > It needs codes. Because the transform is not applied to location of LayoutSVGText. > So if no this codes, should disturb selection. Ah, this is compensating for the reverse thing in LayoutSVGInlineText::positionForPoint... You could write this as AffineTransform::translation(...) or transform.translate(...).
fs, Could you check PS 32? Thank you.
On 2016/01/19 10:44:20, hyunjunekim2 wrote: > fs, Could you check PS 32? Thank you. I updated PS33.
On 2016/01/19 at 10:44:20, hyunjune.kim wrote: > fs, Could you check PS 32? Thank you. I didn't see that all issue from the previous PSs had been addressed yet. https://codereview.chromium.org/1541083002/diff/610001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp (right): https://codereview.chromium.org/1541083002/diff/610001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:455: FloatPoint absolutePoint(pointInContents); Maybe call this "clippedPointInContents" or something instead - no reason to round-trip through FloatPoint here either. Also, add a comment describing why this is being done. https://codereview.chromium.org/1541083002/diff/610001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:456: if (absolutePoint.x() < 0) This operation would be the same as LayoutPoint::clampNegativeToZero. https://codereview.chromium.org/1541083002/diff/610001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/610001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:490: FloatRect boundingBox = child->objectBoundingBox(); Move down. https://codereview.chromium.org/1541083002/diff/610001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:516: FloatPoint absolutePoint = closestLayoutObject->localToParentTransform().inverse().mapPoint(point); Calling this "pointInChild" or "childLocalPoint" like above seems to better describe what it is. https://codereview.chromium.org/1541083002/diff/610001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:526: if (result) You could write this like the previous version before the for-loop, and then just return nullptr at the end instead.
fs, Could you check PS 36? Thank you.
https://codereview.chromium.org/1541083002/diff/690001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/1541083002/diff/690001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:294: if (closestDescendant) You trivially know this (checked 3 lines before.) The toLayoutSVGText cast below should also be asserting this already (and more firmly so.) https://codereview.chromium.org/1541083002/diff/690001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/690001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:481: // Try to search closest LayoutSVGText on sub-tree of `layoutObject`. "Try to find the closest LayoutSVGText in the subtree rooted at |layoutObject|." https://codereview.chromium.org/1541083002/diff/690001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:482: // When closest LayoutObject hasn't LayoutSVGText object, try to search on other LayoutObjects LayoutSVGText's are not treat specially in any way, it's just that if a LayoutSVGText is the closest on this level, then no further search is required. At least that's how the code is currently written. https://codereview.chromium.org/1541083002/diff/690001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:503: if (closestLayoutObject && boundingBox.contains(closestLayoutObject->objectBoundingBox())) { To re-iterate: I don't think that 'overlaps' is a very interesting metric if searching "globally". (Also, this would only be interesting for containers - right? Suggest that the Vector should store only container references.) Maybe it would be easier to just "queue" containers that are closer than any LayoutSVGText and then sort the vector on <distance, child-index> (hopefully using a stable sort means only the first is needed as a sort key.) So a structure like: for (...) { if (child->isSVGText()) { <compute distance> if (distance >= closestDistance) continue; <clear set of containers with potentially closer LayoutSVGTexts> <update distance and closest> continue; } if (child->isSVGContainer() && !child->isSVGHiddenContainer()) { <compute distance> if (distance >= closestDistance) continue; <add to set of containers with potentially closer LayoutSVGTexts> } } ... return LayoutSVGText if closest ... ... sort set of containers by increasing distance ... iterate (containers) recurse into container Does that match your intention by any chance? (The top comment sounds a bit like it.)
> Does that match your intention by any chance? (The top comment sounds a bit
like it.)
I though about many case.
this case, i expected g's text selected.
<svg xmlns="http://www.w3.org/2000/svg" width="800" height="800">
<text x="30" y="60">G Front Element</text>
<g>
<text x="30" y="50">G Rear Element</text>
<rect x="30" y="80" width="100" height="100" fill="green"/>
</g>
</svg>
and another case, i want to give chance to G elements for selecting to their
children.
<svg xmlns="http://www.w3.org/2000/svg" width="800" height="800">
<g>
<text x="30" y="50">G Rear Element</text>
<rect x="30" y="80" width="100" height="100" fill="green"/>
</g>
<g>
<text x="30" y="60">G Front Element</text>
</g>
<text x="400" y="400">Hi, nice to meet you.</text>
</svg>
But i think that your suggestion is reasonable.
I am going to add comments. Thank you.
On 2016/01/20 at 06:26:38, hyunjune.kim wrote: > > Does that match your intention by any chance? (The top comment sounds a bit like it.) > > I though about many case. > > this case, i expected g's text selected. > <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800"> > <text x="30" y="60">G Front Element</text> > <g> > <text x="30" y="50">G Rear Element</text> > <rect x="30" y="80" width="100" height="100" fill="green"/> > </g> > </svg> > > and another case, i want to give chance to G elements for selecting to their children. > <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800"> > <g> > <text x="30" y="50">G Rear Element</text> > <rect x="30" y="80" width="100" height="100" fill="green"/> > </g> > <g> > <text x="30" y="60">G Front Element</text> > </g> > <text x="400" y="400">Hi, nice to meet you.</text> > </svg> Ok, so maybe you want to track both the distance to closest text on this level as well as the distance to the closest container.
On 2016/01/20 13:24:07, fs wrote: > On 2016/01/20 at 06:26:38, hyunjune.kim wrote: > > > Does that match your intention by any chance? (The top comment sounds a bit > like it.) > > > > I though about many case. > > > > this case, i expected g's text selected. > > <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800"> > > <text x="30" y="60">G Front Element</text> > > <g> > > <text x="30" y="50">G Rear Element</text> > > <rect x="30" y="80" width="100" height="100" fill="green"/> > > </g> > > </svg> > > > > and another case, i want to give chance to G elements for selecting to their > children. > > <svg xmlns="http://www.w3.org/2000/svg" width="800" height="800"> > > <g> > > <text x="30" y="50">G Rear Element</text> > > <rect x="30" y="80" width="100" height="100" fill="green"/> > > </g> > > <g> > > <text x="30" y="60">G Front Element</text> > > </g> > > <text x="400" y="400">Hi, nice to meet you.</text> > > </svg> > > Ok, so maybe you want to track both the distance to closest text on this level > as well as the distance to the closest container. Yes, it is. fs, Could you give me feedback about PS39? Thank you.
https://codereview.chromium.org/1541083002/diff/750001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/750001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:481: // Try to search closest |LayoutSVGText| on sub-tree of |layoutObject|. > |LayoutSVGText| |...| is used for variables, so this should just be LayoutSVGText to avoid confusion. Same for all the other places that does this. https://codereview.chromium.org/1541083002/diff/750001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:486: Vector<LayoutObject*> candidates; Make this: struct SearchCandidate { LayoutObject* layoutObject; float distance; }; Vector<SearchCandidate> candidates; and then you don't need to recompute distance for the "secondary" search, and the sort predicate (see below) ought to be simple enough still. https://codereview.chromium.org/1541083002/diff/750001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:488: if (!child->localToParentTransform().isInvertible()) This will return identity for for example LayoutSVGHiddenContainer, so you really want to filter on type first in some way. https://codereview.chromium.org/1541083002/diff/750001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:513: // Sort using the distance between the mouse point and a candidate. In this day and age we have smarty functions that mean we don't need to write sorting code. std::stable_sort ought to work fine here with a suitable predicate.
fs, Could you check PS40? Thank you.
On 2016/01/21 13:38:06, hyunjunekim2 wrote: > fs, Could you check PS40? Thank you. Rebased it.
https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:493: if (child->isSVGHiddenContainer() || !child->localToParentTransform().isInvertible()) Marginally better than before. I think it'd better to explicitly handle the cases we're interested in and "continue" for the rest. https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:500: if (distance >= closestDistance) If distance == closestDistance and !child->isSVGText() don't you want to include the candidate in that case? https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:525: for (SearchCandidate searchCandidate : candidates) { Use reference. https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h (right): https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h:111: struct SearchCandidate { No need to put this in the header. https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h:122: static bool compareCandidateDistance(const SearchCandidate, const SearchCandidate); Ditto. Also, pass arguments as references.
Could you give me your advice? Thank you. https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:493: if (child->isSVGHiddenContainer() || !child->localToParentTransform().isInvertible()) On 2016/01/21 14:27:15, fs wrote: > Marginally better than before. I think it'd better to explicitly handle the > cases we're interested in and "continue" for the rest. I separated it. https://codereview.chromium.org/1541083002/diff/770001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:500: if (distance >= closestDistance) On 2016/01/21 14:27:16, fs wrote: > If distance == closestDistance and !child->isSVGText() don't you want to include > the candidate in that case? I contained it in that case.
Also Could you check PS43?
Please add more tests. https://codereview.chromium.org/1541083002/diff/810001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/810001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:498: // Try to search closest LayoutSVGText on sub-tree of |layoutObject|. Comments could use some touch-up. (I'd prefer not to "micro-manage" them.) https://codereview.chromium.org/1541083002/diff/810001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:507: if (!child->localToParentTransform().isInvertible()) This still makes us do work for example for shapes (which we don't care about here.) If it helps you could put the distance calculation in a helper function. https://codereview.chromium.org/1541083002/diff/810001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:544: if (LayoutObject* result = findClosestLayoutSVGText(candidateLayoutObject, candidateLocalPoint)) Should this be considering all candidates with equal distance regardless of if one LayoutSVGText was found already (with the same original distance)?
fs, Could you check PS44? and I am going to add more tests. Thank you.
https://codereview.chromium.org/1541083002/diff/850001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/850001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:496: static inline bool isPossibleToCalculateDistance(LayoutObject* layoutObject) Yeah, this is not quite what I had in mind when I said "If it helps you could put the distance calculation in a helper function"... More like: static inline float distanceTo...(const LayoutObject* child, const FloatPoint& point) { FloatRect boundingBox = child->objectBoundingBox(); FloatPoint childLocalPoint = child->localToParentTransform().inverse().mapPoint(point); return boundingBox.squaredDistanceTo(childLocalPoint); } or even: static inline float distanceTo...(const LayoutObject* child, const FloatPoint& point) { const AffineTransform& localToParentTransform = child->localToParentTransform(); if (!localToParentTransform.isInvertible()) return std::numeric_limits<float>::max(); FloatPoint childLocalPoint = localToParentTransform.inverse().mapPoint(point); return child->objectBoundingBox().squaredDistanceTo(childLocalPoint); } for an all-in-one package. This function is not really an improvement. Just move the !layoutObject->isSVGHiddenContainer() into the same if condition as child->isSVGContainer().
fs, Could you check PS46? added tests and made a function to calculate distance. Thank you.
https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html (right): https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:27: function verify(start, end) { Rather than copying these helpers to all test, put them in SelectionTestCase.js and reuse. They should preferably get some more descriptive names. I wonder if it wouldn't make for better testing to not tie both of the test points to the same element, so maybe: selectTextFromCharToPoint({ element: 'line', offset: 12 }, { x: ..., y: ... }, ...expected range...); (which folds verify into the same function.) https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:48: public: Not needed (default for 'struct') https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:512: float distance = distanceToChildLayoutObject(child, point); Move this inside both ifs. https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:530: if (closestLayoutObject && closestLayoutObject->isSVGText()) I guess this should only be done when candidates.isEmpty()?
On 2016/01/25 13:00:57, fs wrote: > https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html > (right): > > https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:27: > function verify(start, end) { > Rather than copying these helpers to all test, put them in SelectionTestCase.js > and reuse. They should preferably get some more descriptive names. I wonder if > it wouldn't make for better testing to not tie both of the test points to the > same element, so maybe: > > selectTextFromCharToPoint({ element: 'line', offset: 12 }, { x: ..., y: ... }, > ...expected range...); > > (which folds verify into the same function.) Could you check a test called 'selection-dragging-outside-1.html'? > https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): > > https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:48: public: > Not needed (default for 'struct') > > https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:512: float > distance = distanceToChildLayoutObject(child, point); > Move this inside both ifs. > > https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:530: if > (closestLayoutObject && closestLayoutObject->isSVGText()) > I guess this should only be done when candidates.isEmpty()? I fixed it. fs, Could you give me advice? Thank you.
On 2016/01/25 at 14:54:48, hyunjune.kim wrote: > On 2016/01/25 13:00:57, fs wrote: ... https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:512: float > > distance = distanceToChildLayoutObject(child, point); > > Move this inside both ifs. > > > > https://codereview.chromium.org/1541083002/diff/890001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:530: if > > (closestLayoutObject && closestLayoutObject->isSVGText()) > > I guess this should only be done when candidates.isEmpty()? > I fixed it. Not really. The if you added is redundant. I.e compare: if (closestLayoutObject && candidates.isEmpty()) return closestLayoutObject; // Only set if isSVGText(), so could be of that type even. https://codereview.chromium.org/1541083002/diff/930001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html (right): https://codereview.chromium.org/1541083002/diff/930001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:33: function verify(start, end) { Just include this directly in the above. No need for a separate function. For improved utility I think you'll also need to specify (and check) the start/end element of the range.
fs, Could you check a test which is |selection-dragging-outside-1.html|, and |SVGLayoutSupport.cpp| changed? Thank you.
https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html (right): https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:13: function runTest() { This function could be removed - just inline it into the single call-site. https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:16: selectTextFromCharToPoint({ id: 'line', offset: 12 }, { x: 300, y: 300 }, { start: "12", end: "24" }); You should have a test where the point being moved to is not <300, 300>. https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:529: if (closestLayoutObject && candidates.isEmpty()) Now closestLayoutObject need to be considered somehow in the loop below.
https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:529: if (closestLayoutObject && candidates.isEmpty()) On 2016/01/26 14:24:01, fs wrote: > Now closestLayoutObject need to be considered somehow in the loop below. Do you means somehow which check overlap? Could you give me more a hint?
I think that need to check null. https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:529: if (closestLayoutObject && candidates.isEmpty()) I guess that if ((!closestLayoutObject || closestLayoutObject) && candidates.isEmpty())) return closestLayoutObject;
> if ((!closestLayoutObject || closestLayoutObject) && candidates.isEmpty())) > return closestLayoutObject; It's poor ... -_- i will check it about somehow.
On 2016/01/26 at 14:43:18, hyunjune.kim wrote: > https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): > > https://codereview.chromium.org/1541083002/diff/970001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:529: if (closestLayoutObject && candidates.isEmpty()) > On 2016/01/26 14:24:01, fs wrote: > > Now closestLayoutObject need to be considered somehow in the loop below. > Do you means somehow which check overlap? > > > Could you give me more a hint? Not overlap, but it may not be the closest.
i think just that
if (candidates.isEmpty())
return closestLayoutObject;
On 2016/01/26 at 14:58:12, hyunjune.kim wrote: > i think just that > if (candidates.isEmpty()) > return closestLayoutObject; Yes, that should be equivalent, but that's not the "problem" I'm seeing (which is for the negation of the [old] condition basically).
Description was changed from ========== Not working text-selection when dragging mouse outside the SVG text element. Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. TEST=selection-dragging-outside.html BUG=445329 ========== to ========== Not working text-selection when dragging mouse outside the SVG text element. Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. BUG=445329 ==========
On 2016/01/26 15:01:58, fs wrote: > On 2016/01/26 at 14:58:12, hyunjune.kim wrote: > > i think just that > > if (candidates.isEmpty()) > > return closestLayoutObject; > > Yes, that should be equivalent, but that's not the "problem" I'm seeing (which > is for the negation of the [old] condition basically). fs, I seems that not need sorting. right?
On 2016/01/26 15:14:44, hyunjunekim2 wrote: > On 2016/01/26 15:01:58, fs wrote: > > On 2016/01/26 at 14:58:12, hyunjune.kim wrote: > > > i think just that > > > if (candidates.isEmpty()) > > > return closestLayoutObject; > > > > Yes, that should be equivalent, but that's not the "problem" I'm seeing (which > > is for the negation of the [old] condition basically). > > fs, I seems that not need sorting. right? It's not right.
how do you look?
if (child->isSVGContainer() && !layoutObject->isSVGHiddenContainer()) {
float distance = distanceToChildLayoutObject(child, point);
if (distance > closestDistance)
continue;
closestDistance = distance; // add
candidates.insert(SearchCandidate(child, distance),0);
}
// remove soring.
// over is same.
for(...) {
// find LayoutSVGText.
}
fs, The Concept for finding closest LayoutSVGText is changed from Patch Set 46.
fs, Could you check PS52?
On 2016/01/26 15:30:14, hyunjunekim2 wrote:
> fs, Could you check PS52?
The original concept is the depth-First. If find LayoutSVGText on upper level of
tree, we just return it.
So I change the condition like this.
if (closestLayoutObject)
return closestLayoutObject;
Thank you.
On 2016/01/26 at 15:30:14, hyunjune.kim wrote: > fs, Could you check PS52? I get that feeling that you should slow down, take a step back and ponder what you think should happen. Write some tests w/ <text>s at different/varying depths in the tree and different distances from each other - overlapping and not. Then take some time to think about what to expect for different start and end points during a selection in those. Try to codify that as non-interactive tests and let those tests guide the implementation.
On 2016/01/26 15:40:58, fs wrote: > On 2016/01/26 at 15:30:14, hyunjune.kim wrote: > > fs, Could you check PS52? > > I get that feeling that you should slow down, take a step back and ponder what > you think should happen. Write some tests w/ <text>s at different/varying depths > in the tree and different distances from each other - overlapping and not. Then > take some time to think about what to expect for different start and end points > during a selection in those. Try to codify that as non-interactive tests and let > those tests guide the implementation. Ok, Do you means that slowly landing because need more thinking?
On 2016/01/26 at 15:47:14, hyunjune.kim wrote: > On 2016/01/26 15:40:58, fs wrote: > > On 2016/01/26 at 15:30:14, hyunjune.kim wrote: > > > fs, Could you check PS52? > > > > I get that feeling that you should slow down, take a step back and ponder what > > you think should happen. Write some tests w/ <text>s at different/varying depths > > in the tree and different distances from each other - overlapping and not. Then > > take some time to think about what to expect for different start and end points > > during a selection in those. Try to codify that as non-interactive tests and let > > those tests guide the implementation. > > Ok, Do you means that slowly landing because need more thinking? No need to slowly land anything (I hope), but it'll help (both of us) to know what to expect.
fs, I wrote a report about survey for 1 months.
Survey about PS53
Priority to search
1) Low depth first
2) Physical distance between mouse position and LayoutSVGText
On the basis of the above rule, select LayoutSVGText. This is your suggestion on
PS36.
For example,
<svg>
<g><text id="temp1" x="10" y="10"></text></g>
<text id="temp2" x="100" y="100"></text>
</svg>
If there is on (5, 5) which is mouse position, select a text called "temp2".
This case is `1)` which is selection rule.
Another example,
<svg>
<g id="g1" translate="transform(100,100)"><text id="temp1" x="10"
y="10"></text></g>
<g id="g2" translate="transform(200,200)"><text id="temp1" x="10"
y="10"></text></g>
</svg>
If there is on (5, 5) which is mouse position, select a text of <g> called "g1".
This case is '2)'.
One months have seen worrying to solve the problem completely. Two thought.
First) Exhaustive search
Second) Quad Tree(https://en.wikipedia.org/wiki/Quadtree) or K-D
Tree(https://en.wikipedia.org/wiki/K-d_tree)
But Sencod is some difficult to implement algorithms to search space.
So i guess that PS53 which suggest on PS36 is reasonable.
Thank you for your help.
On 2016/01/27 at 13:49:12, hyunjune.kim wrote: > fs, I wrote a report about survey for 1 months. > Survey about PS53 > > Priority to search > > 1) Low depth first > 2) Physical distance between mouse position and LayoutSVGText I'm not sure if "low depth" should be a priority. > On the basis of the above rule, select LayoutSVGText. This is your suggestion on PS36. AFAICR, my only suggestion has been to try to prune the search (reduce the search-space). I believe I suggested that BFS might be better for that reason, but I'd be willing to concede on that if it makes the code simpler. In general though: my suggestions are not law, and I can be convinced that other solutions are better (preferably by code.) > For example, > <svg> > <g><text id="temp1" x="10" y="10"></text></g> > <text id="temp2" x="100" y="100"></text> > </svg> > > If there is on (5, 5) which is mouse position, select a text called "temp2". > This case is `1)` which is selection rule. > > Another example, > <svg> > <g id="g1" translate="transform(100,100)"><text id="temp1" x="10" y="10"></text></g> > <g id="g2" translate="transform(200,200)"><text id="temp1" x="10" y="10"></text></g> > </svg> > If there is on (5, 5) which is mouse position, select a text of <g> called "g1". > This case is '2)'. See above. > One months have seen worrying to solve the problem completely. Two thought. > > First) Exhaustive search > Second) Quad Tree(https://en.wikipedia.org/wiki/Quadtree) or K-D Tree(https://en.wikipedia.org/wiki/K-d_tree) > > But Sencod is some difficult to implement algorithms to search space. > So i guess that PS53 which suggest on PS36 is reasonable. Quad/KD tree (or other spatial structures) will not really help (because they need to be built.) To re-iterate what I said above, what I've suggested is a pruned exhaustive search.
I am thinking like this. I will upload next patch. https://codereview.chromium.org/1541083002/diff/1050001/third_party/WebKit/So... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/1050001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:535: Like this i will try again. this is just pseudo codes. if (closestLayoutObject && candidates.isEmpty()) return SearchCandidate(closestLayoutObject, closetDistance); std::stable_sort(candidates.begin(), candidates.end(), compareCandidateDistance); SearchCandidate closestCandidate(closestLayoutObject ? closestLayoutObject : nullptr, closestLayoutObject ? closetDistance : std::numeric_limits<float>::max()); for (SearchCandidate& searchCandidate : candidates) { LayoutObject* candidateLayoutObject = searchCandidate.candidateLayoutObject; if (closestLayout.distance < candidateLayoutObject.distace) return closestCandidate.candidateLayoutObject; FloatPoint candidateLocalPoint = candidateLayoutObject->localToParentTransform().inverse().mapPoint(point); SearchCandidate result = findClosestLayoutSVGText(candidateLayoutObject, candidateLocalPoint); if (result.distance < closestCandidate.distance) closestCandidate = result; }
fs, Could you check |searchTreeForFindClosestLayoutSVGText| concept on PS58? Thank you.
https://codereview.chromium.org/1541083002/diff/1130001/third_party/WebKit/So... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/1130001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:534: for (SearchCandidate& searchCandidate : candidates) { We should find the closest LayoutSVGText on condidates. https://codereview.chromium.org/1541083002/diff/1130001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:535: if (closestSearchCandidateText.distance < searchCandidate.distance) We are not all traversal. If |closestSearchCandidateText| that is LayoutSVGText is close then |searchCandidate| that is group, get out |for| using break.
On 2016/02/07 at 10:45:36, hyunjune.kim wrote: > fs, Could you check |searchTreeForFindClosestLayoutSVGText| concept on PS58? Thank you. Looks like a reasonable approach.
Description was changed from ========== Not working text-selection when dragging mouse outside the SVG text element. Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. BUG=445329 ========== to ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. BUG=445329 ==========
Description was changed from ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because on LayoutReplaced::position- ForPoint, if has node(), return either createPositionWith- Affinity(0) or createPositionWithAffinity(1). So After re-define positionForPoint on the SVG Root, and implement selection logic. Find LayoutSVGText near by mouse-point, and calculate PositionWithAffinity. BUG=445329 ========== to ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. BUG=445329 ==========
Description was changed from ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. BUG=445329 ========== to ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1). BUG=445329 ==========
Description was changed from ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1). BUG=445329 ========== to ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. BUG=445329 ==========
Description was changed from ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. BUG=445329 ========== to ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on the SVGRootElement, and implement text selection. BUG=445329 ==========
Description was changed from ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on the SVGRootElement, and implement text selection. BUG=445329 ========== to ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on the SVGRootElement, and implement text selection. BUG=445329 ==========
Description was changed from ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on the SVGRootElement, and implement text selection. BUG=445329 ========== to ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on the SVGRootElement, and implement text selection. BUG=445329 ==========
Description was changed from ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on the SVGRootElement, and implement text selection. BUG=445329 ========== to ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on the LayoutSVGRoot, and implement text selection. BUG=445329 ==========
Description was changed from ========== Implement text selection on SVGRootElement. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on the LayoutSVGRoot, and implement text selection. BUG=445329 ========== to ========== Implement text selection on |LayoutSVGRoot|. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on |LayoutSVGRoot|, and implement text selection. BUG=445329 ==========
Description was changed from ========== Implement text selection on |LayoutSVGRoot|. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on LayoutReplaced::positionForPoint. So After redefine |positionForPoint| on |LayoutSVGRoot|, and implement text selection. BUG=445329 ========== to ========== Implement text selection on |LayoutSVGRoot|. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. So After redefine |positionForPoint| on |LayoutSVGRoot|, and implement text selection. BUG=445329 ==========
Description was changed from ========== Implement text selection on |LayoutSVGRoot|. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. So After redefine |positionForPoint| on |LayoutSVGRoot|, and implement text selection. BUG=445329 ========== to ========== Implement text selection on |LayoutSVGRoot|. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. So after redefine |positionForPoint| on |LayoutSVGRoot|, and implement text selection. BUG=445329 ==========
Description was changed from ========== Implement text selection on |LayoutSVGRoot|. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. So after redefine |positionForPoint| on |LayoutSVGRoot|, and implement text selection. BUG=445329 ========== to ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. So after redefine |positionForPoint| on LayoutSVGRoot, and implement text selection. BUG=445329 ==========
fs, Could you check PS61? Thank you.
https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/resources/SelectionTestCase.js (right): https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/resources/SelectionTestCase.js:146: shouldBe("range.startOffset", expectedRange.start); You should check for an expected element here too. (For both of the end-points.) https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-2.html (right): https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-2.html:34: endPos = element.getEndPositionOfChar(10); This test would be easier to follow if you wrapped up the common bits in functions. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:48: SearchCandidate() {} Maybe this should set the layoutObject memeer to nullptr and the distance to max() as you do below? https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:49: SearchCandidate(LayoutObject* layoutObject, float dist) dist -> distance https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:54: LayoutObject* candidateLayoutObject; The 'candidate'-prefix seems redundant. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:519: float closestDistance = std::numeric_limits<float>::max(); Maybe put these two in a SearchCandidate too. Maybe you could even move closestSearchCandidateText to here instead. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:522: // First find LayoutSVGText on the this tree level and also find candidates. "Find the closest LayoutSVGText on this tree level, and also collect any containers that could contain LayoutSVGTexts that are closer." https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:542: // If find LayoutSVGText and don't have candidates, just return |closestLayoutObject|. "If a LayoutSVGText was found and there are no potentially closer sub-trees, just return ..." https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:548: SearchCandidate closestSearchCandidateText = SearchCandidate(nullptr, std::numeric_limits<float>::max()); SearchCandidate closestSearchCandidateText(nullptr, ...) but also see suggestion above. You could probably also shorten the name a bit without losing readability - maybe just 'closestText'? https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:549: // Find the closest LayoutSVGText on |condidates| that is sub-trees. "on |condidates| that is sub-trees." -> "in the sub-trees in |candidates|." (or similar) https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:550: // If find layoutSVGText on |searchCandidate| and it is closer then other |searchCandidate|, stop iteration statement. "If a LayoutSVGText is found that is (strictly) closer than any previous candidate, then end the search." https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:551: for (SearchCandidate& searchCandidate : candidates) { const https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:563: // Compare distance between |closestLayoutObject| and |closestSearchCandidateText|. "Compare" is pretty obvious, maybe "Return the closest of ..." https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:572: SearchCandidate result = searchTreeForFindClosestLayoutSVGText(layoutObject, point); Could just do: return searchTree...(...).candidateLayoutObject;
fs, Could you check PS63? Thank you. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/resources/SelectionTestCase.js (right): https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/resources/SelectionTestCase.js:146: shouldBe("range.startOffset", expectedRange.start); On 2016/02/12 17:05:42, fs wrote: > You should check for an expected element here too. (For both of the end-points.) Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-2.html (right): https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-2.html:34: endPos = element.getEndPositionOfChar(10); On 2016/02/12 17:05:42, fs wrote: > This test would be easier to follow if you wrapped up the common bits in > functions. Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right): https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:48: SearchCandidate() {} On 2016/02/12 17:05:42, fs wrote: > Maybe this should set the layoutObject memeer to nullptr and the distance to > max() as you do below? Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:49: SearchCandidate(LayoutObject* layoutObject, float dist) On 2016/02/12 17:05:43, fs wrote: > dist -> distance Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:54: LayoutObject* candidateLayoutObject; On 2016/02/12 17:05:42, fs wrote: > The 'candidate'-prefix seems redundant. Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:519: float closestDistance = std::numeric_limits<float>::max(); On 2016/02/12 17:05:42, fs wrote: > Maybe put these two in a SearchCandidate too. Maybe you could even move > closestSearchCandidateText to here instead. Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:522: // First find LayoutSVGText on the this tree level and also find candidates. On 2016/02/12 17:05:43, fs wrote: > "Find the closest LayoutSVGText on this tree level, and also collect any > containers that could contain LayoutSVGTexts that are closer." Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:542: // If find LayoutSVGText and don't have candidates, just return |closestLayoutObject|. On 2016/02/12 17:05:42, fs wrote: > "If a LayoutSVGText was found and there are no potentially closer sub-trees, > just return ..." Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:548: SearchCandidate closestSearchCandidateText = SearchCandidate(nullptr, std::numeric_limits<float>::max()); On 2016/02/12 17:05:42, fs wrote: > SearchCandidate closestSearchCandidateText(nullptr, ...) > > but also see suggestion above. You could probably also shorten the name a bit > without losing readability - maybe just 'closestText'? Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:549: // Find the closest LayoutSVGText on |condidates| that is sub-trees. On 2016/02/12 17:05:42, fs wrote: > "on |condidates| that is sub-trees." -> "in the sub-trees in |candidates|." (or > similar) Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:550: // If find layoutSVGText on |searchCandidate| and it is closer then other |searchCandidate|, stop iteration statement. On 2016/02/12 17:05:42, fs wrote: > "If a LayoutSVGText is found that is (strictly) closer than any previous > candidate, then end the search." Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:551: for (SearchCandidate& searchCandidate : candidates) { On 2016/02/12 17:05:42, fs wrote: > const Done. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:563: // Compare distance between |closestLayoutObject| and |closestSearchCandidateText|. On 2016/02/12 17:05:42, fs wrote: > "Compare" is pretty obvious, maybe "Return the closest of ..." Done. Remove if statement. https://codereview.chromium.org/1541083002/diff/1190001/third_party/WebKit/So... third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:572: SearchCandidate result = searchTreeForFindClosestLayoutSVGText(layoutObject, point); On 2016/02/12 17:05:42, fs wrote: > Could just do: > > return searchTree...(...).candidateLayoutObject; Done.
Almost there https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/resources/SelectionTestCase.js (right): https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/resources/SelectionTestCase.js:157: Nit: Unnecessary blank line. https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html (right): https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:2: <svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="400" height="400" style="border: 1px solid black;"> Nit/Pro-tip: You don't need 'version' or any of the NS-declarations (the former because it's useless - the latter because they're implicitly added by the HTML parser.) (This applies to all the tests) https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:4: <text x="130" y="30" font-family="Arial" font-size="10" fill="black">Happy, Web</text> Specifying the style for the <text>s in either a <style> or on <svg> would make the tests less cluttered. (This applies to all the tests) https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:15: selectTextFromCharToPoint({ id: 'line', offset: 12 }, { x: 300, y: 300 }, { startElementId: "line", start: "12", endElementId: "line", end: "24" }); Maybe also add some case where selection is started at the top and then progressively moved down along a vertical line. I.e something like: ...({ id: 'firsttext', offset: 0 }, { x: <somevalue within the texts>, y: 20 }, ...); ...({ id: 'firsttext', offset: 0 }, { x: <somevalue within the texts>, y: 30 }, ...); ...({ id: 'firsttext', offset: 0 }, { x: <somevalue within the texts>, y: 40 }, ...); ...({ id: 'firsttext', offset: 0 }, { x: <somevalue within the texts>, y: 50 }, ...); ... https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-3.html (right): https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-3.html:8: <text id="line1" x="10" y="70" font-family="Arial" font-size="10" fill="black">Hello World. Hello, SVG.</text> Maybe consider using text content of differing lengths so that moving up/down does not does not guarantee a "hit". Also, inconsisten indent of these two <text>s (4 vs. 2)
fs, Could you give me advice about vertical align? Thank you. https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/resources/SelectionTestCase.js (right): https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/resources/SelectionTestCase.js:157: On 2016/02/15 14:11:24, fs wrote: > Nit: Unnecessary blank line. Done. https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html (right): https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:2: <svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="400" height="400" style="border: 1px solid black;"> On 2016/02/15 14:11:24, fs wrote: > Nit/Pro-tip: You don't need 'version' or any of the NS-declarations (the former > because it's useless - the latter because they're implicitly added by the HTML > parser.) > > (This applies to all the tests) Done. https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:4: <text x="130" y="30" font-family="Arial" font-size="10" fill="black">Happy, Web</text> On 2016/02/15 14:11:24, fs wrote: > Specifying the style for the <text>s in either a <style> or on <svg> would make > the tests less cluttered. > > (This applies to all the tests) Done. https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:15: selectTextFromCharToPoint({ id: 'line', offset: 12 }, { x: 300, y: 300 }, { startElementId: "line", start: "12", endElementId: "line", end: "24" }); fs, I used writing-mode: tb; for vertical. ref to 'text-selection-align-05-b.svg' and 'text-selection-align-06-b.svg'. But it has bug don't select text also inside text. Could you give me advice? https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-3.html (right): https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-3.html:8: <text id="line1" x="10" y="70" font-family="Arial" font-size="10" fill="black">Hello World. Hello, SVG.</text> On 2016/02/15 14:11:24, fs wrote: > Maybe consider using text content of differing lengths so that moving up/down > does not does not guarantee a "hit". > > Also, inconsisten indent of these two <text>s (4 vs. 2) Done.
> https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... > third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:15: > selectTextFromCharToPoint({ id: 'line', offset: 12 }, { x: 300, y: 300 }, { > startElementId: "line", start: "12", endElementId: "line", end: "24" }); fs, I used writing-mode: tb; for vertical. ref to 'text-selection-align-05-b.svg' and 'text-selection-align-06-b.svg'. But it has bug don't select text also inside text. Could you give me advice?
On 2016/02/15 at 16:13:26, hyunjune.kim wrote: > > https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... > > third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:15: > > selectTextFromCharToPoint({ id: 'line', offset: 12 }, { x: 300, y: 300 }, { > > startElementId: "line", start: "12", endElementId: "line", end: "24" }); > > fs, I used writing-mode: tb; for vertical. ref to > 'text-selection-align-05-b.svg' and 'text-selection-align-06-b.svg'. > But it has bug don't select text also inside text. > Could you give me advice? I'm not sure I understand what you mean. Do you mean that the actual vertical text (the words 'start' , 'middle', 'end') is not possible to select? If so, I see that on master/trunk too, so it's not your fault (but we should probably file a bug about it.)
Could you check PS65? And Could you give me guide to create new issue? now push `new issue` button on 'http://code.google.com /p/chromium/issues', redirect invalid page that say 'This wizard can only be used with code.google.com and bugs.chromium.org.'. Thank you. https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... File third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html (right): https://codereview.chromium.org/1541083002/diff/1230001/third_party/WebKit/La... third_party/WebKit/LayoutTests/svg/text/selection-dragging-outside-1.html:15: selectTextFromCharToPoint({ id: 'line', offset: 12 }, { x: 300, y: 300 }, { startElementId: "line", start: "12", endElementId: "line", end: "24" }); On 2016/02/15 16:08:12, hyunjunekim2 wrote: > fs, I used writing-mode: tb; for vertical. ref to > 'text-selection-align-05-b.svg' and 'text-selection-align-06-b.svg'. > But it has bug don't select text also inside text. > Could you give me advice? Done.
> And Could you give me guide to create new issue? > now push `new issue` button on 'http://code.google.com > /p/chromium/issues', redirect invalid page that say > 'This wizard can only be used with http://code.google.com and bugs.chromium.org.'. Ok, I see monorail(https://bugs.chromium.org/p/monorail/issues/list).
fs, I have updated PS65. Thank you.
> So after redefine |positionForPoint| on LayoutSVGRoot, and implement text selection. I'd suggest you reword and expand on this part of the commit message. (You didn't really "implement text selection", but more a way of finding the LayoutSVGText closest to a point.) Maybe: "Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point." And preferably more than that. Otherwise LGTM.
Description was changed from ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. So after redefine |positionForPoint| on LayoutSVGRoot, and implement text selection. BUG=445329 ========== to ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAff- inity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAff- inity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAff- inity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAff- inity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAff- inity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement text selection on LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAff- inity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAff- inity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAff- inity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAff- inity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAff- inity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Description was changed from ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
On 2016/02/17 16:32:35, fs wrote: > > So after redefine |positionForPoint| on LayoutSVGRoot, and implement text > selection. > > I'd suggest you reword and expand on this part of the commit message. (You > didn't really "implement text selection", but more a way of finding the > LayoutSVGText closest to a point.) > > Maybe: > > "Implement positionForPoint() for LayoutSVGRoot and implement a way to find the > LayoutSVGText closest to the point." > > And preferably more than that. > > Otherwise LGTM. fs, Done. Could you check it? Thank you.
On 2016/02/18 at 01:57:15, hyunjune.kim wrote: > On 2016/02/17 16:32:35, fs wrote: > > > So after redefine |positionForPoint| on LayoutSVGRoot, and implement text > > selection. > > > > I'd suggest you reword and expand on this part of the commit message. (You > > didn't really "implement text selection", but more a way of finding the > > LayoutSVGText closest to a point.) > > > > Maybe: > > > > "Implement positionForPoint() for LayoutSVGRoot and implement a way to find the > > LayoutSVGText closest to the point." > > > > And preferably more than that. > > > > Otherwise LGTM. > > fs, Done. Could you check it? 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/1541083002/1270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1541083002/1270001
Message was sent while issue was closed.
Description was changed from ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ==========
Message was sent while issue was closed.
Committed patchset #65 (id:1270001)
Message was sent while issue was closed.
Description was changed from ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 ========== to ========== Implement positionForPoint() for LayoutSVGRoot. Not working text-selection when dragging mouse outside the SVG text element. Because return either createPositionWithAffinity(0) or createPositionWithAffinity(1) on |LayoutReplaced::positionForPoint|. Implement positionForPoint() for LayoutSVGRoot and implement a way to find the LayoutSVGText closest to the point. BUG=445329 Committed: https://crrev.com/22889144f57acb143ddf25576f9ae309e6d88b13 Cr-Commit-Position: refs/heads/master@{#376141} ==========
Message was sent while issue was closed.
Patchset 65 (id:??) landed as https://crrev.com/22889144f57acb143ddf25576f9ae309e6d88b13 Cr-Commit-Position: refs/heads/master@{#376141} |
