WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
114872
[GTK] Reimplement atk_text_get_text_*_offset for LINE boundaries
https://bugs.webkit.org/show_bug.cgi?id=114872
Summary
[GTK] Reimplement atk_text_get_text_*_offset for LINE boundaries
Mario Sanchez Prada
Reported
2013-04-19 06:50:25 PDT
We need to get rid of Pango/Gail dependency, and this is one of the functionalities we would need to fix.
Attachments
Patch proposal
(11.85 KB, patch)
2013-07-16 10:11 PDT
,
Mario Sanchez Prada
eflews.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
(683.74 KB, application/zip)
2013-07-16 12:04 PDT
,
Build Bot
no flags
Details
Patch proposal
(11.79 KB, patch)
2013-07-19 10:08 PDT
,
Mario Sanchez Prada
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from APPLE-EWS-6 for win-future
(107.90 KB, application/zip)
2013-07-25 23:59 PDT
,
Build Bot
no flags
Details
Patch proposal
(10.21 KB, patch)
2013-08-03 08:19 PDT
,
Mario Sanchez Prada
no flags
Details
Formatted Diff
Diff
Patch proposal
(13.77 KB, patch)
2013-08-09 09:54 PDT
,
Mario Sanchez Prada
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Patch proposal
(13.77 KB, patch)
2013-09-04 03:20 PDT
,
Mario Sanchez Prada
gustavo
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Mario Sanchez Prada
Comment 1
2013-07-16 10:11:52 PDT
Created
attachment 206794
[details]
Patch proposal I tried this patch, besides passing all the unit and layout tests, it is the first one in this series that fixes a real and important issue we currently have in WebKit2GTK+: not able to navigate line by line with ATs (see
bug 73433
). So, kindly asking for review over it
Mario Sanchez Prada
Comment 2
2013-07-16 10:12:26 PDT
I'm basing this patch on the one for the WORD boundary, thus reflecting that in the dependency tree
EFL EWS Bot
Comment 3
2013-07-16 10:14:55 PDT
Comment on
attachment 206794
[details]
Patch proposal
Attachment 206794
[details]
did not pass efl-ews (efl): Output:
http://webkit-queues.appspot.com/results/1090279
kov's GTK+ EWS bot
Comment 4
2013-07-16 10:17:30 PDT
Comment on
attachment 206794
[details]
Patch proposal
Attachment 206794
[details]
did not pass gtk-ews (gtk): Output:
http://webkit-queues.appspot.com/results/1085265
EFL EWS Bot
Comment 5
2013-07-16 10:19:47 PDT
Comment on
attachment 206794
[details]
Patch proposal
Attachment 206794
[details]
did not pass efl-wk2-ews (efl-wk2): Output:
http://webkit-queues.appspot.com/results/1089247
Build Bot
Comment 6
2013-07-16 12:04:13 PDT
Comment on
attachment 206794
[details]
Patch proposal
Attachment 206794
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/1080282
New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html
Build Bot
Comment 7
2013-07-16 12:04:17 PDT
Created
attachment 206803
[details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Mario Sanchez Prada
Comment 8
2013-07-19 10:08:36 PDT
Created
attachment 207116
[details]
Patch proposal New patch after some changes I did while working on the SENTENCE boundary
EFL EWS Bot
Comment 9
2013-07-19 10:12:29 PDT
Comment on
attachment 207116
[details]
Patch proposal
Attachment 207116
[details]
did not pass efl-ews (efl): Output:
http://webkit-queues.appspot.com/results/1105873
EFL EWS Bot
Comment 10
2013-07-19 10:13:44 PDT
Comment on
attachment 207116
[details]
Patch proposal
Attachment 207116
[details]
did not pass efl-wk2-ews (efl-wk2): Output:
http://webkit-queues.appspot.com/results/1130760
kov's GTK+ EWS bot
Comment 11
2013-07-19 10:15:03 PDT
Comment on
attachment 207116
[details]
Patch proposal
Attachment 207116
[details]
did not pass gtk-ews (gtk): Output:
http://webkit-queues.appspot.com/results/1098993
Build Bot
Comment 12
2013-07-25 23:59:10 PDT
Comment on
attachment 207116
[details]
Patch proposal
Attachment 207116
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/1248150
New failing tests: dom/xhtml/level2/events/dispatchEvent03.xhtml dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg dom/html/level2/events/dispatchEvent04.html dom/xhtml/level2/events/dispatchEvent02.xhtml dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg dom/html/level1/core/hc_attrappendchild4.html dom/xhtml/level2/core/createAttributeNS06.xhtml dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html dom/svg/level3/xpath/XPathEvaluator_evaluate_NOT_SUPPORTED_ERR.svg dom/xhtml/level2/events/dispatchEvent04.xhtml dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg dom/html/level2/core/setAttributeNS10.html dom/xhtml/level2/events/dispatchEvent01.xhtml dom/html/level2/events/dispatchEvent03.html dom/html/level2/events/dispatchEvent02.html dom/html/level2/core/createDocumentType04.html dom/xhtml/level2/events/dispatchEvent05.xhtml dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html dom/html/level1/core/hc_attrappendchild2.html dom/html/level2/events/dispatchEvent06.html dom/html/level2/events/dispatchEvent07.html dom/html/level2/events/dispatchEvent01.html dom/html/level2/events/dispatchEvent05.html dom/html/level2/core/createAttributeNS06.html dom/xhtml/level2/events/dispatchEvent06.xhtml dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html dom/html/level2/core/hc_namednodemapinvalidtype1.html
Build Bot
Comment 13
2013-07-25 23:59:13 PDT
Created
attachment 207511
[details]
Archive of layout-test-results from APPLE-EWS-6 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: APPLE-EWS-6 Port: win-future Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
Mario Sanchez Prada
Comment 14
2013-08-03 08:19:29 PDT
Created
attachment 208065
[details]
Patch proposal Patch rebased against trunk, to make EWS happy
Mario Sanchez Prada
Comment 15
2013-08-09 09:54:21 PDT
Created
attachment 208440
[details]
Patch proposal I just realized that I can actually uncomment some tests that were previously commented due to a bug in GailTextUtil, so I'm attaching a new patch now including that.
Build Bot
Comment 16
2013-08-22 00:48:31 PDT
Comment on
attachment 208440
[details]
Patch proposal
Attachment 208440
[details]
did not pass win-ews (win): Output:
http://webkit-queues.appspot.com/results/1499377
Mario Sanchez Prada
Comment 17
2013-09-04 03:20:04 PDT
Created
attachment 210441
[details]
Patch proposal New patch renaming new function webkitAccessibleTextGetLineForBoundary to webkitAccessibleTextLineForBoundary for consistency with the rest of the code (see recently landed patch for
bug 114873
). This is now the last patch in the "remove pango/gail dependency from ATK code" series :)
chrys
Comment 18
2013-09-05 05:06:39 PDT
Is there a chance to get it in the 2.2? I m really looking to this :)
Mario Sanchez Prada
Comment 19
2013-09-10 02:00:18 PDT
(In reply to
comment #18
)
> Is there a chance to get it in the 2.2? I m really looking to this :)
Stating the obvious, we need to go through the review process first. Once that's done, you can believe me I'll be the first one interested in getting it in, along with other patches that are currently depending on this one :)
Brian Holt
Comment 20
2013-09-10 07:44:45 PDT
Comment on
attachment 210441
[details]
Patch proposal View in context:
https://bugs.webkit.org/attachment.cgi?id=210441&action=review
Informal review: looks sane to me, just a few suggestions and queries
> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:982 > + // Additionally to checking that we are not at the end of a block, we need
Additionally -> In addition
> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:990 > + if (isEndOfLine(position))
Ternary operator? startPosition = isEndOfline(position) ? position : logicalStartOfLine(position);
> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1016 > + Document* document = coreObject->document();
Check coreObject == 0?
> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1026 > + // Besides of the usual conversion from ATK offsets to WebCore offsets,
"of" can be removed
> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1038 > + case GetTextPositionAt:
Is there nothing here on purpose? Maybe a comment then?
> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1061 > + VisibleSelection selectedLine = newPosition != caretPosition ? lineAtPositionForAtkBoundary(coreObject, newPosition, boundaryType) : currentLine;
parentheses around the condition being evaluated?
Gustavo Noronha (kov)
Comment 21
2013-09-10 07:54:21 PDT
Comment on
attachment 210441
[details]
Patch proposal View in context:
https://bugs.webkit.org/attachment.cgi?id=210441&action=review
LGTM
> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1026 > + // Besides of the usual conversion from ATK offsets to WebCore offsets,
s/of //
Mario Sanchez Prada
Comment 22
2013-09-10 08:22:00 PDT
Comment on
attachment 210441
[details]
Patch proposal View in context:
https://bugs.webkit.org/attachment.cgi?id=210441&action=review
Thanks both for your reviews. I will address those issues before landing the patch
>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:982 >> + // Additionally to checking that we are not at the end of a block, we need > > Additionally -> In addition
Ok.
>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:990 >> + if (isEndOfLine(position)) > > Ternary operator? > startPosition = isEndOfline(position) ? position : logicalStartOfLine(position);
Yes, I could use that. I just didn't do it for consistency with similar usages in this same file (where readability would be worse if I used it), but in this case I can certainly use it.
>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1016 >> + Document* document = coreObject->document(); > > Check coreObject == 0?
Not needed. Any AtkObject will always have a AccessibilityObject, so we normally don't do check for those objects to be different than NULL unless we are walking up or down through the hierarchy.
>>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1026 >>> + // Besides of the usual conversion from ATK offsets to WebCore offsets, >> >> "of" can be removed > > s/of //
That's a typo. Or not. We'll never know, but I'll remove it anyway :D
>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1038 >> + case GetTextPositionAt: > > Is there nothing here on purpose? Maybe a comment then?
Yes, there's nothing there because the AT position is the default case, hence we do not need to do any additional calculation such as getting the text after or before the line boundaries we previously found. At the same time we want to have this case here so we can catch later any other invalid position type that might have been passed here (even if unlikely) in the default switch case below. In any case, a comment here would be helpful. I'll add one before landing
>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1061 >> + VisibleSelection selectedLine = newPosition != caretPosition ? lineAtPositionForAtkBoundary(coreObject, newPosition, boundaryType) : currentLine; > > parentheses around the condition being evaluated?
We don't normally add them if they are not strictly needed from a logical pov.
Mario Sanchez Prada
Comment 23
2013-09-11 03:31:07 PDT
Committed
r155516
: <
http://trac.webkit.org/changeset/155516
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug