WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
92818
REGRESSION: WebKit does not render selection in non-first ruby text nodes.
https://bugs.webkit.org/show_bug.cgi?id=92818
Summary
REGRESSION: WebKit does not render selection in non-first ruby text nodes.
Sukolsak Sakshuwong
Reported
2012-07-31 18:18:57 PDT
Created
attachment 155696
[details]
test case Reproduction steps: 1. Create a page with the following contents: <!DOCTYPE html> <html> <head> <meta charset="utf-8"> </head> <body> a<ruby>一<rt>1</rt>二<rt>2</rt>三<rt>3</rt></ruby>b </body> </html> 2. Press Cmd+A (or Ctrl+A on Windows/Linux) to select all Expected result: "a 一1 二2 三3 b" is highlighted Actual result: Only "a 一1 二 三 b" is highlighted
Attachments
test case
(138 bytes, text/html)
2012-07-31 18:18 PDT
,
Sukolsak Sakshuwong
no flags
Details
Patch
(19.99 KB, patch)
2012-07-31 22:37 PDT
,
Sukolsak Sakshuwong
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-07
(365.65 KB, application/zip)
2012-07-31 23:13 PDT
,
WebKit Review Bot
no flags
Details
Patch
(35.46 KB, patch)
2012-12-12 22:51 PST
,
Ryosuke Niwa
leviw
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Elliott Sprehn
Comment 1
2012-07-31 19:19:41 PDT
This doesn't happen in Safari 5.1, but happens in ToT (and Chrome 20, etc.).
Sukolsak Sakshuwong
Comment 2
2012-07-31 22:37:32 PDT
Created
attachment 155722
[details]
Patch
Ryosuke Niwa
Comment 3
2012-07-31 22:59:39 PDT
Comment on
attachment 155722
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=155722&action=review
Unfortunately, I'm not qualified to review this patch :(
> Source/WebCore/ChangeLog:14 > + No new tests (OOPS!).
You need to mention the test you're adding here. See other change log entries for an example.
WebKit Review Bot
Comment 4
2012-07-31 23:13:46 PDT
Comment on
attachment 155722
[details]
Patch
Attachment 155722
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/13415076
New failing tests: fast/ruby/select-ruby.html
WebKit Review Bot
Comment 5
2012-07-31 23:13:50 PDT
Created
attachment 155725
[details]
Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Robert Hogan
Comment 6
2012-08-01 13:49:38 PDT
Comment on
attachment 155722
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=155722&action=review
> Source/WebCore/ChangeLog:12 > + RenderRubyRun is an 'inline-block/table'-like object and therefore should be > + a selection root. However, because it's anonymous, the isSelectionRoot() method > + returned false. This caused RootInlineBox::selectionTopAdjustedForPrecedingBlock > + to return incorrect values and caused the highlight to not show up. > + Make isSelectionRoot() return true for anonymous inline-block objects.
Driveby comment: To cater for the RubyRun case you're opening up isSelectionRoot() to all inline blocks and tables. This might be fine, but I think you need more tests to show that's the case.
Sukolsak Sakshuwong
Comment 7
2012-08-01 14:48:25 PDT
(In reply to
comment #3
)
> > Source/WebCore/ChangeLog:14 > > + No new tests (OOPS!). > > You need to mention the test you're adding here. See other change log entries for an example.
Will fix. (In reply to
comment #6
)
> > Source/WebCore/ChangeLog:12 > > + RenderRubyRun is an 'inline-block/table'-like object and therefore should be > > + a selection root. However, because it's anonymous, the isSelectionRoot() method > > + returned false. This caused RootInlineBox::selectionTopAdjustedForPrecedingBlock > > + to return incorrect values and caused the highlight to not show up. > > + Make isSelectionRoot() return true for anonymous inline-block objects. > > Driveby comment: To cater for the RubyRun case you're opening up isSelectionRoot() to all inline blocks and tables. This might be fine, but I think you need more tests to show that's the case.
Thanks for the comment. Since isSelectionRoot() has already returned true for all non-anonymous inline-block objects, I just opened it up for anonymous inline-block objects as well. Do we have anonymous inline-blocks objects (or anonymous inline tables) in WebKit other than RubyRun?
Levi Weintraub
Comment 8
2012-08-22 11:37:22 PDT
(In reply to
comment #7
)
> > Driveby comment: To cater for the RubyRun case you're opening up isSelectionRoot() to all inline blocks and tables. This might be fine, but I think you need more tests to show that's the case. > > Thanks for the comment. Since isSelectionRoot() has already returned true for all non-anonymous inline-block objects, I just opened it up for anonymous inline-block objects as well. Do we have anonymous inline-blocks objects (or anonymous inline tables) in WebKit other than RubyRun?
Seems like this could happen with Tables. Julien can probably tell you.
Julien Chaffraix
Comment 9
2012-08-22 12:29:01 PDT
(In reply to
comment #8
)
> (In reply to
comment #7
) > > > Driveby comment: To cater for the RubyRun case you're opening up isSelectionRoot() to all inline blocks and tables. This might be fine, but I think you need more tests to show that's the case. > > > > Thanks for the comment. Since isSelectionRoot() has already returned true for all non-anonymous inline-block objects, I just opened it up for anonymous inline-block objects as well. Do we have anonymous inline-blocks objects (or anonymous inline tables) in WebKit other than RubyRun? > > Seems like this could happen with Tables. Julien can probably tell you.
We can't have anonymous inline tables at this time as we haven't implemented this (see
bug 15365
, the bottom line is that it's non-trivial). Beware that it is expected and something we will want to fix someday though.
Radar WebKit Bug Importer
Comment 10
2012-12-12 00:59:49 PST
<
rdar://problem/12862809
>
Ryosuke Niwa
Comment 11
2012-12-12 15:26:58 PST
Comment on
attachment 155722
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=155722&action=review
> Source/WebCore/rendering/RenderBlock.cpp:3189 > if (!node()) > return false;
I've done some investigation and it appears that we should just remove this line. It was added in
http://trac.webkit.org/changeset/12986
but I don't think we need it anymore.
Elliott Sprehn
Comment 12
2012-12-12 15:33:18 PST
Comment on
attachment 155722
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=155722&action=review
> Source/WebCore/rendering/RenderBlock.cpp:3185 > + if (isInlineBlockOrInlineTable())
This permits :before and :after to be selection roots, was that intended?
>> Source/WebCore/rendering/RenderBlock.cpp:3189 >> return false; > > I've done some investigation and it appears that we should just remove this line. It was added in
http://trac.webkit.org/changeset/12986
but I don't think we need it anymore.
Removing this line will make PseudoElement's capable of being selection roots. Note that now the code is if (!nonPseudoNode()) return; it'll also make the anonymous renderers inside them, and :first-letter, etc. all into selection roots.
Ryosuke Niwa
Comment 13
2012-12-12 22:51:56 PST
Created
attachment 179203
[details]
Patch
WebKit Review Bot
Comment 14
2012-12-13 02:03:03 PST
Comment on
attachment 179203
[details]
Patch
Attachment 179203
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/15311175
New failing tests: editing/selection/range-between-block-and-inline.html fast/repaint/selection-clear.html fast/ruby/select-ruby.html
Levi Weintraub
Comment 15
2012-12-27 12:41:14 PST
Comment on
attachment 179203
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=179203&action=review
> Source/WebCore/ChangeLog:11 > + it doesn't lay down its children in block direction.
Lay down == lay out?
> Source/WebCore/ChangeLog:30 > +
http://trac.webkit.org/changeset/12986
but all tests added by this change set as well as the rest > + of layout tests pass without this condition.
Sadness :(
Ryosuke Niwa
Comment 16
2013-01-23 16:34:26 PST
Committed
r140613
: <
http://trac.webkit.org/changeset/140613
>
Elliott Sprehn
Comment 17
2013-01-23 16:45:59 PST
(In reply to
comment #15
)
> (From update of
attachment 179203
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=179203&action=review
> > > Source/WebCore/ChangeLog:11 > > + it doesn't lay down its children in block direction. > > Lay down == lay out? > > > Source/WebCore/ChangeLog:30 > > +
http://trac.webkit.org/changeset/12986
but all tests added by this change set as well as the rest
This linked changeset is wrong, it's from 7 years ago. I don't think this patch is right though. The original check was: if (!node()) return; I changed it to be: if (!node() || isPseudoElment()) return; in
https://trac.webkit.org/changeset/136744
and then I encapsulated that to: if (!nonPseudoNode) return; in
https://trac.webkit.org/changeset/137336
So this patch just enabled all anonymous RenderBlock (and all subclasses) as being selection roots which doesn't seem right without more tests. Also the ASSERT(node() || isAnonymous()); is a truism. Every renderer is either anonymous, or has a node.
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