| Summary: | unicode-bidi:plaintext is supposed to be effective on display:inline elements too | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Aharon (Vladimir) Lanin <aharon> | ||||||||||||||||||
| Component: | CSS | Assignee: | Levi Weintraub <leviw> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | aharon, eric, leviw, mitz, playmobil, rniwa, webkit.review.bot, xji | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Bug Depends on: | 69267, 78092 | ||||||||||||||||||||
| Bug Blocks: | 50910 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Aharon (Vladimir) Lanin
2011-11-29 04:53:23 PST
Created attachment 116949 [details]
Test case (although should really be converted to a ref test)
Do you men that both isolate and plaintext should work for inline element? Created attachment 117049 [details] Work in progress This currently crashes due, at least in part, to bug 69267. Created attachment 117083 [details]
Patch
This is still dependent upon 69267, and fails the included relayout test case. It's still a step in the right direction :)
Comment on attachment 117083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117083&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:74 > + #writer.write_image_diff_files(image_diff) This was done to get Ref Tests to not crash on the mac port. It won't be in the final patch :) (In reply to comment #2) > Do you men that both isolate and plaintext should work for inline element? Yes. I personally don't see much point in using plaintext on an inline element, but the now-specified behavior makes sense if someone does use it that way. Comment on attachment 117083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117083&action=review > Source/WebCore/rendering/InlineIterator.h:134 > + if (unicodeBidi == Isolate || unicodeBidi == Plaintext) { Can we add isIsolate() to where EUnicodeBidi.h is defined? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:957 > // It does not matter which order we resolve the runs as long as we resolve them all. Doesn't your approach make the order to matter here? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:968 > + determineDirectionality(direction, InlineIterator(isolatedInline, (firstIsolatedRunOnLine) ? topResolver.firstIsolatedObjectOnLine() : bidiFirstSkippingEmptyInlines(isolatedInline), 0)); Don't need parentheses around firstIsolatedRunOnLine. > LayoutTests/fast/text/international/inline-plaintext-is-isolated-expected.html:1 > +<!DOCTYPE HTML> Remove BOM? > LayoutTests/fast/text/international/inline-plaintext-relayout-with-leading-neutrals-expected.html:1 > +<!DOCTYPE HTML> Ditto. > LayoutTests/fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html:1 > +<!DOCTYPE HTML> Ditto. > LayoutTests/fast/text/international/inline-plaintext-with-generated-content-expected.html:1 > +<!DOCTYPE HTML> Ditto. > LayoutTests/fast/text/international/inline-plaintext-with-generated-content.html:1 > +<!DOCTYPE HTML> Ditto. Comment on attachment 117083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117083&action=review >> Source/WebCore/rendering/InlineIterator.h:134 >> + if (unicodeBidi == Isolate || unicodeBidi == Plaintext) { > > Can we add isIsolate() to where EUnicodeBidi.h is defined? For sure. This cleans things up. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:957 >> // It does not matter which order we resolve the runs as long as we resolve them all. > > Doesn't your approach make the order to matter here? No. Each isolated run is still independent. >> LayoutTests/fast/text/international/inline-plaintext-is-isolated-expected.html:1 >> +<!DOCTYPE HTML> > > Remove BOM? Good catch, thanks. Created attachment 119937 [details]
Patch
Comment on attachment 119937 [details] Patch Attachment 119937 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10915221 Created attachment 119949 [details]
Patch
Comment on attachment 119949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119949&action=review > Source/WebCore/ChangeLog:22 > + (WebCore::BidiResolver::BidiResolver): Added state for tracking the first object on a line > + after a hard line break in a bidi-isolated RenderInline. This is needed to keep track of where > + to start creating TextRuns in constructBidiRuns. It'll be helpful if you had an example for which this state is needed in the change log. > Source/WebCore/ChangeLog:45 > +2011-12-19 Levi Weintraub <leviw@chromium.org> > + > + unicode-bidi:plaintext is supposed to be effective on display:inline elements too > + https://bugs.webkit.org/show_bug.cgi?id=73310 > + > + Reviewed by NOBODY (OOPS!). change log dup. > Source/WebCore/rendering/InlineIterator.h:466 > + addPlaceholderRunForIsolatedInline(resolver, iter.object(), iter.offset()); > + resolver.setBeginningOfIsolatedRunOnLine(iter); Starting http://trac.webkit.org/changeset/102875, fake runs stores the offset in the object instead of just the object. Could you explain why we need to store a separate iterator in addition to what we already have in the changelog? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:977 > + else > + isolatedInline = toRenderInline(containingIsolate(isolatedRun->object(), currentRoot)); We should probably assert that unicodeBidi is Isolate in this case. Created attachment 120082 [details]
Patch
Ping reviewers. Another ping for a review. Created attachment 123843 [details]
Patch
(In reply to comment #16) > Created an attachment (id=123843) [details] > Patch Updating the patch to trunk. Eric, Ryosuke, anyone have some cycles to spare for a review? Comment on attachment 123843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123843&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1672 > - // FIXME: Why does "unicode-bidi: plaintext" bidiFirstIncludingEmptyInlines when all other line layout code uses bidiFirstSkippingEmptyInlines? Why? Otherwise the patch looks OK. Maybe levi answered my question before and I forgot? Comment on attachment 123843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123843&action=review >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1672 >> - // FIXME: Why does "unicode-bidi: plaintext" bidiFirstIncludingEmptyInlines when all other line layout code uses bidiFirstSkippingEmptyInlines? > > Why? D'oh!! When I updated, this was the one conflict. I removed the FIXME because in the previous version of the patch, I'd made this use bidiFirstSkippingEmptyInlines as the comment suggests it should. Let me fix that. Created attachment 123856 [details]
Patch
(In reply to comment #19) > Otherwise the patch looks OK. Maybe levi answered my question before and I forgot? Thanks for the good catch. The new patch should fix things. Comment on attachment 123856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123856&action=review > Source/WebCore/rendering/InlineIterator.h:257 > +static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver = 0) So why is this resolver=0 support needed? Comment on attachment 123856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123856&action=review >> Source/WebCore/rendering/InlineIterator.h:257 >> +static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver = 0) > > So why is this resolver=0 support needed? Specifically for the callsite in determineStartPosition that used to be bidiFirstIncludingEmptyInlines. It doesn't have a resolver that should be modified. Comment on attachment 123856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123856&action=review >>> Source/WebCore/rendering/InlineIterator.h:257 >>> +static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver = 0) >> >> So why is this resolver=0 support needed? > > Specifically for the callsite in determineStartPosition that used to be bidiFirstIncludingEmptyInlines. It doesn't have a resolver that should be modified. So we have this strange scenerio where we need to determine the start position in order to determine the direction, and then we need the direction in order to configure the resolver as part of determining the start position? Am I understanding that correctly? Is there a better way we should be refactoring this (not necessarily in this patch)? Comment on attachment 123856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123856&action=review >>>> Source/WebCore/rendering/InlineIterator.h:257 >>>> +static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver = 0) >>> >>> So why is this resolver=0 support needed? >> >> Specifically for the callsite in determineStartPosition that used to be bidiFirstIncludingEmptyInlines. It doesn't have a resolver that should be modified. > > So we have this strange scenerio where we need to determine the start position in order to determine the direction, and then we need the direction in order to configure the resolver as part of determining the start position? Am I understanding that correctly? Is there a better way we should be refactoring this (not necessarily in this patch)? Effectively yes. We want to position an Inline Iterator at the first position to run the generic UBA algorithm to determine the directionality of the paragraph (first strong character). When in plaintext, we use this to seed the resolver. The misnomer for me here is "determineStartPosition," as it clearly is doing a lot more than determining the start position for line layout; it's also setting a lot of state. Perhaps this could be cleared up with a bidiFirst that doesn't take a resolver, and that bidiFirstSkippingEmptyInlines uses. Then we could keep the assert and dump the if. (In reply to comment #26) > Perhaps this could be cleared up with a bidiFirst that doesn't take a resolver, and that bidiFirstSkippingEmptyInlines uses. Then we could keep the assert and dump the if. How does this sound for a refactoring? Want me to do that in this patch? (In reply to comment #27) > (In reply to comment #26) > > Perhaps this could be cleared up with a bidiFirst that doesn't take a resolver, and that bidiFirstSkippingEmptyInlines uses. Then we could keep the assert and dump the if. > > How does this sound for a refactoring? Want me to do that in this patch? This actually isn't really possible without duplicating some code. I could create a simple inline function that better describes the unique use case here and simply maps into bidiFirstSkippingEmptyInlines with no resolver. Comment on attachment 123856 [details]
Patch
OK.
Comment on attachment 123856 [details] Patch Clearing flags on attachment: 123856 Committed r107000: <http://trac.webkit.org/changeset/107000> All reviewed patches have been landed. Closing bug. |