Bug 50126 - Fallback content in canvas element should be focusable and accessible
: Fallback content in canvas element should be focusable and accessible
Status: RESOLVED DUPLICATE of bug 124592
Product: WebKit
Classification: Unclassified
Component: Canvas
: 528+ (Nightly build)
: PC Windows Vista
: P2 Normal
Assigned To: Dominic Mazzoni
:
Depends on: 87898 87899 91794
Blocks: 54697
  Show dependency treegraph
 
Reported: 2010-11-27 11:44 PST by Charles Pritchard
Modified: 2013-11-19 11:55 PST (History)
30 users (show)

See Also:


Attachments
Proposed patch to add support for accessible canvas fallback content. Includes new layout test. (9.04 KB, patch)
2011-02-09 13:12 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Updated patch responds to Chris's review (9.72 KB, patch)
2011-02-11 15:44 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Updated patch now adds new canvas role and fixes style error. (15.14 KB, patch)
2011-02-15 07:55 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Updated patch modifies failing layout test to recognize new role for canvas elements. (16.62 KB, patch)
2011-02-15 12:26 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (30.31 KB, patch)
2012-04-23 04:13 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (963.59 KB, application/zip)
2012-04-23 19:33 PDT, WebKit Review Bot
no flags Details
Patch (58.78 KB, patch)
2012-05-10 12:14 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (788.19 KB, application/zip)
2012-05-11 03:32 PDT, WebKit Review Bot
no flags Details
Patch (75.74 KB, patch)
2012-05-29 02:03 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (599.89 KB, application/zip)
2012-05-29 09:43 PDT, WebKit Review Bot
no flags Details
Patch (90.28 KB, patch)
2012-05-30 02:21 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (543.57 KB, application/zip)
2012-05-30 05:30 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Pritchard 2010-11-27 11:44:14 PST
According to the HTML Canvas specs at the W3C, fallback content such as [input] should be focusable via element.focus(). At this point, in WebKit, it's not.

"When a canvas element represents embedded content, the user can still focus descendants of the canvas element (in the fallback content). This allows authors to make an interactive canvas keyboard-focusable: authors should have a one-to-one mapping of interactive regions to focusable elements in the fallback content."
Comment 1 Eric Seidel 2011-01-24 13:25:54 PST
This bug needs a reduction/test case and should be updated with links to the relevants spec(s) as well as information about what other browsers do.
Comment 2 Charles Pritchard 2011-01-27 09:59:16 PST
http://www.w3.org/TR/html5/the-canvas-element.html
("the user can still focus")

Here are two tests, one fires a focus event on the element, on load, the other should fire an event when the tab key is pressed (and tab index navigated to).

[body onload="document.getElementById('test').focus()"]
[canvas]
[input type="checkbox" id="test" onfocus="alert('You found me')" tabindex="1" /]
[/canvas]
[/body]


Bug also posted on Chromium:
http://code.google.com/p/chromium/issues/detail?id=64683

Currently, Internet Explorer 9 behaves per the standard, Firefox 4 does not (yet, I believe), but has started work on it.
Comment 3 Eric Seidel 2011-01-27 13:27:43 PST
The test case would be better as an attachment.

Good to know IE/FF behavior.  Do we have a FF bug link?  (Not necessary, bug possibly helpful.)
Comment 4 Charles Pritchard 2011-01-31 12:00:26 PST
From David Bolter: "closest bug"

https://bugzilla.mozilla.org/show_bug.cgi?id=495912
Bug 495912 - Expose alternative content in Canvas element to ATs
Comment 5 Ryosuke Niwa 2011-02-04 21:03:17 PST
This might be to do with editingIgnoresContent and canHaveChildrenForEditing.
Comment 6 Dominic Mazzoni 2011-02-09 13:12:41 PST
Created attachment 81856 [details]
Proposed patch to add support for accessible canvas fallback content. Includes new layout test.
Comment 7 chris fleizach 2011-02-09 23:07:47 PST
Comment on attachment 81856 [details]
Proposed patch to add support for accessible canvas fallback content. Includes new layout test.

View in context: https://bugs.webkit.org/attachment.cgi?id=81856&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3283
>      

The default for allowing children is true. I don't think canvas fits any of the roles that don't allow children, so it seems this is not necessary

> Source/WebCore/html/HTMLFormControlElement.cpp:243
>          return false;

Instead of removing the size check, which may be valid in other contexts, should you just add another check to determine if its size is empty and it's not a child of a canvas area.

> Source/WebCore/rendering/RenderObject.cpp:1317
>  }

Might be a good idea to have a comment why this changed
Comment 8 Dominic Mazzoni 2011-02-11 15:43:17 PST
Comment on attachment 81856 [details]
Proposed patch to add support for accessible canvas fallback content. Includes new layout test.

View in context: https://bugs.webkit.org/attachment.cgi?id=81856&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3283
>>      
> 
> The default for allowing children is true. I don't think canvas fits any of the roles that don't allow children, so it seems this is not necessary

The layout test doesn't pass without this line. A canvas has a roleValue of ImageRole, so the switch statement immediately below this causes it to return false (that it's not allowed to have children).

Is there a better role for canvas? Should we create a new role (internally) but then map it to Image at the platform-specific accessibility API level?

>> Source/WebCore/html/HTMLFormControlElement.cpp:243
>>          return false;
> 
> Instead of removing the size check, which may be valid in other contexts, should you just add another check to determine if its size is empty and it's not a child of a canvas area.

Good idea, done.

>> Source/WebCore/rendering/RenderObject.cpp:1317
>>  }
> 
> Might be a good idea to have a comment why this changed

The issue is that clippedOverflowRectForRepaint(repaintContainer) tries to access the containing block and crashes if it doesn't exist. A child of a RenderHTMLCanvas doesn't necessarily have a containing block. Luckily repainting isn't needed for that case, either, so it suffices to just check whether containingBlock() exists.

I restructured it slightly and added a comment, how's this?
Comment 9 Dominic Mazzoni 2011-02-11 15:44:09 PST
Created attachment 82201 [details]
Updated patch responds to Chris's review
Comment 10 WebKit Review Bot 2011-02-11 15:47:26 PST
Attachment 82201 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1

Source/WebCore/html/HTMLFormControlElement.cpp:257:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 chris fleizach 2011-02-11 16:41:00 PST
(In reply to comment #8)
> (From update of attachment 81856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81856&action=review
> 
> >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3283
> >>      
> > 
> > The default for allowing children is true. I don't think canvas fits any of the roles that don't allow children, so it seems this is not necessary
> 
> The layout test doesn't pass without this line. A canvas has a roleValue of ImageRole, so the switch statement immediately below this causes it to return false (that it's not allowed to have children).
> 
> Is there a better role for canvas? Should we create a new role (internally) but then map it to Image at the platform-specific accessibility API level?
>

I should have guessed canvas defaults to an image. I think that we should have a new internal Canvas role that defaults to the Group role on most platforms. I know on the mac, that if an image has a child, VoiceOver will not access it correctly. 
 
> >> Source/WebCore/html/HTMLFormControlElement.cpp:243
> >>          return false;
> > 
> > Instead of removing the size check, which may be valid in other contexts, should you just add another check to determine if its size is empty and it's not a child of a canvas area.
> 
> Good idea, done.
> 
> >> Source/WebCore/rendering/RenderObject.cpp:1317
> >>  }
> > 
> > Might be a good idea to have a comment why this changed
> 
> The issue is that clippedOverflowRectForRepaint(repaintContainer) tries to access the containing block and crashes if it doesn't exist. A child of a RenderHTMLCanvas doesn't necessarily have a containing block. Luckily repainting isn't needed for that case, either, so it suffices to just check whether containingBlock() exists.
> 
> I restructured it slightly and added a comment, how's this?

Looks good. Need to fix the style issue as well
Comment 12 Dominic Mazzoni 2011-02-15 07:55:37 PST
Created attachment 82455 [details]
Updated patch now adds new canvas role and fixes style error.

I like this; it's cleaner to have a separate role for Canvas internally. I mapped the new canvas role to "grouping" as you suggested (and took a guess for the GTK port). Please make sure I didn't miss any spots where I should handle the new role.
Comment 13 chris fleizach 2011-02-15 07:57:43 PST
Comment on attachment 82455 [details]
Updated patch now adds new canvas role and fixes style error.

looks good. will r+ when it passes enough bots
Comment 14 chris fleizach 2011-02-15 09:15:24 PST
Comment on attachment 82455 [details]
Updated patch now adds new canvas role and fixes style error.

r=me
Comment 15 WebKit Commit Bot 2011-02-15 11:13:25 PST
Comment on attachment 82455 [details]
Updated patch now adds new canvas role and fixes style error.

Rejecting attachment 82455 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2

Last 500 characters of output:


=== BUILD AGGREGATE TARGET All OF PROJECT DumpRenderTree WITH CONFIGURATION Release ===
Check dependencies
** BUILD SUCCEEDED **

Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /mnt/git/webkit-commit-queue/LayoutTests
Testing 22607 test cases.
accessibility .............................
accessibility/canvas.html -> failed

Exiting early after 1 failures. 29 tests run.
0.75s total testing time

28 test cases (96%) succeeded
1 test case (3%) had incorrect layout

Full output: http://queues.webkit.org/results/7907978
Comment 16 Dominic Mazzoni 2011-02-15 12:26:01 PST
Created attachment 82504 [details]
Updated patch modifies failing layout test to recognize new role for canvas elements.

Since AXGroup is the role of a lot of things other than just canvas, I modified the test to check the title also, to make sure we're checking the role of the right element.
Comment 17 Charles Pritchard 2011-02-16 20:22:25 PST
!!!! :-)


Thanks Chris.
Comment 18 WebKit Commit Bot 2011-02-16 20:56:18 PST
Comment on attachment 82504 [details]
Updated patch modifies failing layout test to recognize new role for canvas elements.

Clearing flags on attachment: 82504

Committed r78789: <http://trac.webkit.org/changeset/78789>
Comment 19 WebKit Commit Bot 2011-02-16 20:56:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2011-02-17 00:50:23 PST
http://trac.webkit.org/changeset/78789 might have broken GTK Linux 64-bit Debug
Comment 21 Mario Sanchez Prada 2011-02-17 01:39:52 PST
In LayoutTests/accessibility/canvas-fallback-content.html:

[...]
+            var fallback = document.getElementById("fallback");
+            fallback.focus();
+
+            var accessibilityFocusedElement = accessibilityController.focusedElement;
+            
+            var pattern = "AXRole: AXCheckBox";
+            if (document.activeElement.id == "fallback" &&
+                accessibilityController.focusedElement.allAttributes().indexOf(pattern) != -1) {
+                 result.innerText += "Test passed\n";
+            }
+            else {
+                 result.innerText += "Test failed\n" + fallbackElement.allAttributes();
+            }

[...]


Shouldn't the test use 'accessibilityFocusedElement' instead of 'fallbackElement' in the 'else' branch?

The GTK bots are entering in the 'else' branch and printing the following failure in the output:

--- /home/slave/webkitgtk/gtk-linux-32-debug/build/layout-test-results/accessibility/canvas-fallback-content-expected.txt	2011-02-16 22:06:01.629666022 -0800
+++ /home/slave/webkitgtk/gtk-linux-32-debug/build/layout-test-results/accessibility/canvas-fallback-content-actual.txt	2011-02-16 22:06:01.629666022 -0800
@@ -1,3 +1,3 @@
+CONSOLE MESSAGE: line 41: ReferenceError: Can't find variable: fallbackElement
   
-Test passed

(see http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r78789%20(13902)/accessibility/canvas-fallback-content-diffs.txt)


Another doubt I have is whether we could use 'accessibilityFocusedElement' instead of calling  'accessibilityController.focusedElement' again in the second part of the AND in the if condition. Guess so, but not sure whether perhaps there's a good reason for that. Just wondering.

I didn't file a bug for these issues since I'd like to get some confirmation from you first, just to confirm I'm right.


In the other hand, another issue is why the Gtk port enters that 'else' branch, and that's what I'll work on right away. In the meanwhile, I will skip the test in the GTK port, since even if we fixed that variable name the GTK bots would keep the test failing because execution would enter through the else branch anyway...
Comment 22 Mario Sanchez Prada 2011-02-17 01:46:11 PST
(In reply to comment #21)
> [...]
> In the other hand, another issue is why the Gtk port enters that 'else' branch, 
> and that's what I'll work on right away. In the meanwhile, I will skip the test 
> in the GTK port, since even if we fixed that variable name the GTK bots would 
> keep the test failing because execution would enter through the else branch 
> anyway...

In case you're interested:
https://bugs.webkit.org/show_bug.cgi?id=54626
Comment 23 Mario Sanchez Prada 2011-02-17 03:04:44 PST
Yet another thought on this test

(In reply to comment #21)
> In LayoutTests/accessibility/canvas-fallback-content.html:
> 
> [...]
> +            var fallback = document.getElementById("fallback");
> +            fallback.focus();
> +
> +            var accessibilityFocusedElement = accessibilityController.focusedElement;
> +            
> +            var pattern = "AXRole: AXCheckBox";
                                      ^^^^^^^^^^

As far as I now the actual name of the accessibility roles depends on the platform (e.g. Mac: "AXCheckBox" / Atk: "check box"), so perhaps it would be a good idea to place the expected files for these kind of tests under the platform/ directory.

I know there are already many tests printing AX roles not in the platform/ directory, but those are not failing since they're in the Skipped file at the moment. But at some point it would be good to get them fixed and then we'll have the same issue than now, so I guess we could consider doing this for new tests from now on, unless I'm missing something of course :-)

Another approach would be to overwrite the general behavior by "adding" another expected file in platform/port when it differs from the general expected file, but as it seems this does not only affect to Mac and GTK, I think the other approach would be better.

Opinions?
Comment 24 Mario Sanchez Prada 2011-02-17 03:09:08 PST
(In reply to comment #23)
> [...]
> Opinions?

It came to me in a flash that another approach, to leave the expected files in the general directory untouched could be to somehow implement a way to map WebCore/Mac rolenames into Atk rolenames in GTK's DRT, so we always get the same rolenames, regardless of the platform being used.

That wouldn't fix the problem for other platforms that might suffer from it, but the solution there would be similar (WebCore->Qt mapping, WebCore->Win mapping...)

My two cents after brainstorming with myself. Eager to get some feedback...
Comment 25 chris fleizach 2011-02-17 07:48:11 PST
I've also been thinking we need more platform agnostic information checks. Unfortunately that sometimes then does't test end to end on a platform. It would also be nice if in DRT you could detect the platform and apply the right result there sometimes
Comment 26 Mario Sanchez Prada 2011-02-18 02:10:35 PST
(In reply to comment #25)
> I've also been thinking we need more platform agnostic information checks. 

Yep, that's my idea as well. My point is that, for the very specific case of rolenames, if we consider that the rolenames in WebCore are exactly the same than in Mac (I'm kinda guessing here, not 100% sure about this) when the Mac port wouldn't need to do anything additional and would be each port "who" would need to map those WebCore rolenames to the platform-specific ones.

> Unfortunately that sometimes then does't test end to end on a platform.

Didn't understand this sentence. Could you rephrase it? (perhaps it's my English skills, but I have the feeling there's something missing in there :-))

> It would also be nice if in DRT you could detect the platform and apply
> the right result there sometimes

Not sure I follow... DRT is specific fro every platform already
Comment 27 Abhishek Arya 2011-02-28 13:22:48 PST
This introduced security bug - https://bugs.webkit.org/show_bug.cgi?id=55393

Reduced testcase::
    <feOffset>
        <canvas>
            <legend id="test">
                <input/>
            </legend>
        </canvas>
    </feOffset>
    <script>
        window.setTimeout(function() {
            document.getElementById('test').innerHTML = 1;
        }, 0);
    </script>

Stack:
 WebCore::RenderObject::localToAbsolute(WebCore::FloatPoint localPoint=(0,0), bool fixed=false, bool useTransforms=true)  Line 1945 + 0x15 bytes    C++
>WebCore::LayoutState::LayoutState(WebCore::RenderObject * root=0x06e7600c)  Line 121	C++
 WebCore::RenderView::pushLayoutState(WebCore::RenderObject * root=0x06e7600c)  Line 719 + 0x28 bytes    C++
 WebCore::FrameView::layout(bool allowSubtree=true)  Line 900    C++
 WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView> * __formal=0x0a094608)  Line 1604    C++
 WebCore::Timer<WebCore::FrameView>::fired()  Line 100 + 0x29 bytes    C++
 WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 112 + 0xf bytes    C++
 WebCore::ThreadTimers::sharedTimerFired()  Line 91    C++
 webkit_glue::WebKitClientImpl::DoTimeout()  Line 86 + 0xa bytes    C++

LayoutState::LayoutState(RenderObject* root)
    : m_clipped(false)
    , m_pageLogicalHeight(0)
    , m_pageLogicalHeightChanged(false)
    , m_columnInfo(0)
    , m_next(0)
#ifndef NDEBUG
    , m_renderer(root)
#endif
{
    RenderObject* container = root->container();

root is freed.

Comment #5 From Dave Hyatt 2011-02-28 13:13:59 PST (-) [reply] 
(From update of attachment 84094 [details])
I think we should consider reverting the original patch.  I don't think it really should have received r+.  The original changes were bizarre and not reviewed by a layout expert.
Comment 28 Dave Hyatt 2011-02-28 13:23:59 PST
I think the approach taken here was just all wrong.  It would probably be better to (when fallback content is present) create an anonymous RenderBlock that fits the RenderHTMLCanvas snugly and also holds the fallback content and that can become the HTML element's renderer.

That way you don't have to add hack upon hack to try to turn the RenderHTMLCanvas into a container, and you establish a proper containing block for the content.

This patch really should be reverted.
Comment 29 Dave Hyatt 2011-02-28 13:48:32 PST
The other option would be to turn RenderHTMLCanvas into a RenderBlock subclass.  Then it acts as a proper container (and containing block), and you have a lots of control over what you can do to the children.

For example:

(1) You can set overflow:hidden when fallback content is present to prevent layout overflow.
(2) You can make the canvas block act as a containing block for all children regardless of their positioning by patching container() and containingBlock().
(3) You can make the canvas have a layer and establish a stacking context when fallback content is present.
(4) Force visibility:hidden down the descendant tree and you don't even have to patch painting and hit testing.

canvas * { visibility:hidden !important }
Comment 30 Dave Hyatt 2011-02-28 14:22:16 PST
Backing up even further, who says we even have to render this content?  It just has to be focusable.  That's it, right?  Let's just make it focusable without creating any renderers.  That should be easy, right?
Comment 31 James Robinson 2011-02-28 14:42:59 PST
IRC snippet:

<dhyatt>
anyway if we have to implement it and make renderers
[2:37pm]
enrica_ joined the chat room.
[2:37pm] <dhyatt>
the best way to fix it is to make RenderHTMLCanvas a RenderBlock and more like a form control
[2:37pm] <dhyatt>
i.e., inline-block by default, replaced bit set when inline
[2:37pm] <dhyatt>
etc.
[2:38pm]
tonikitoo left the chat room. (Read error: Operation timed out)
[2:38pm] <dhyatt>
not to try to make a RenderReplaced able to have arbitrary child content
[2:38pm] <dhyatt>
that way lies madness
[2:38pm]
diegohcg left the chat room. (Ping timeout: 240 seconds)
[2:38pm] <dhyatt>
RenderReplaced is not designed for that



I also think we should separate the render tree changes from the AX side changes and make sure that experts in each review each side in isolation.
Comment 32 James Robinson 2011-02-28 20:18:02 PST
Marking reopened since the patch was reverted.  Upon further reflection I'm pretty doubtful that this is a good idea.
Comment 33 Abhishek Arya 2011-03-01 13:02:25 PST
(In reply to comment #32)
> Marking reopened since the patch was reverted.  Upon further reflection I'm pretty doubtful that this is a good idea.

Please make sure that regression crasher test from c#27 is landed too when new patch goes.
Comment 34 Eric Seidel 2011-04-06 10:47:43 PDT
Comment on attachment 82455 [details]
Updated patch now adds new canvas role and fixes style error.

Cleared Chris Fleizach's review+ from obsolete attachment 82455 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 35 Charles Pritchard 2011-08-30 17:36:23 PDT
Do we have any new takers on this issue? It's likely that this issue will also crop up with css generated and replaced content. This issue has been implemented in IE9.

Example in css: [div style="content: url('image.png') replaced;"][input tabindex=0 type="submit" /][/div]
Example in canvas: [canvas][input tabindex=0 type="submit" /][/div]

In both cases, the user should be able to tab focus onto the input element, which would fire a focus event on that element.
Comment 36 Maciej Stachowiak 2012-02-13 18:50:22 PST
(In reply to comment #30)
> Backing up even further, who says we even have to render this content?  It just has to be focusable.  That's it, right?  Let's just make it focusable without creating any renderers.  That should be easy, right?

Fr this specific bug, there's no need to have renderers. In generally, we need to expose the canvas's child content to assistive technologies, possibly even including metrics. For some of that, the content may need renderers. I have not yet looked into it in depth, but it would probably be wise to review all the things needed for canvas child DOM accessibility to choose the right approach.
Comment 37 Ryosuke Niwa 2012-02-13 19:20:25 PST
I'm coming at this bug from a slightly different angle here and would like to suggest that we might be better off implementing canvas' default behavior as a shadow DOM. That way, we just replace the fallback content with a shadow DOM in the normal behavior, and will use the original DOM for fallback. And we already have a proven infrastructure for this.
Comment 38 Ryosuke Niwa 2012-02-14 09:48:17 PST
Sorry, maybe my comment was too terse.

What I meant is that we can get rid of the RenderReplaced and use the shadow DOM to implement replaced elements instead. For example, in the case of the cavas element, we'll attach a special shadow DOM that "implements the canvas" by default. The fallback content is shown iff the shadow DOM isn't attached and the innate DOM is shown.

HTMLCanvasElement -> ShadowRoot -> some-special-element/renderer, inaccessible from ES5
|
v
fallback contents (accessible from DOM, focus, etc... would just work when shown)

That way, we don't need to have all sorts of crazy hacks we have in RenderReplaced and friends today.
Comment 39 Dimitri Glazkov (Google) 2012-02-14 09:51:08 PST
(In reply to comment #38)
> Sorry, maybe my comment was too terse.
> 
> What I meant is that we can get rid of the RenderReplaced and use the shadow DOM to implement replaced elements instead. For example, in the case of the cavas element, we'll attach a special shadow DOM that "implements the canvas" by default. The fallback content is shown iff the shadow DOM isn't attached and the innate DOM is shown.
> 
> HTMLCanvasElement -> ShadowRoot -> some-special-element/renderer, inaccessible from ES5
> |
> v
> fallback contents (accessible from DOM, focus, etc... would just work when shown)
> 
> That way, we don't need to have all sorts of crazy hacks we have in RenderReplaced and friends today.

Formulating RenderReplaced as an effect of shadow DOM is a pretty brilliant idea, Ryosuke.  I am probably not aware of all of the edge cases here, but it seems this just needs to be done.
Comment 40 Dimitri Glazkov (Google) 2012-02-14 10:03:44 PST
Filed bug 78616 to track Niwa-san's idea.
Comment 41 Dominic Mazzoni 2012-04-23 04:13:10 PDT
Created attachment 138323 [details]
Patch
Comment 42 WebKit Review Bot 2012-04-23 04:14:56 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 43 Build Bot 2012-04-23 04:36:09 PDT
Comment on attachment 138323 [details]
Patch

Attachment 138323 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12477752
Comment 44 Dominic Mazzoni 2012-04-23 04:42:14 PDT
OK, here's an ugly-but-working patch to continue the conversation. No need to do a line-by-line review yet, I'd just like to figure out what general approach will be the next step.

1. The patch I just uploaded implements Dave Hyatt's suggestion to make RenderHTMLCanvas subclass RenderBlock. In doing so, I copied some code from RenderReplaced; if we decide to go this route, we should figure out how to share that code. The advantage of this approach is that the vast majority of the changes are only in RenderHTMLCanvas; once you make it a RenderBlock, focusable and accessible fallback content happens basically automatically.

2. Another alternative (suggested by Maciej) would be to not create RenderObjects for the fallback content but to still allow them to be focusable. This would be a larger refactoring of accessibility code to create AccessibilityObjects from Elements instead of RenderObjects, but might require fewer changes to RenderHTMLCanvas. Does this approach sound better? Would this actually work even in unusual corner cases like if someone puts an iframe inside of a canvas?

3. How does Ryosuke's shadow dom idea relate to these two approaches? I don't quite understand.
Comment 45 Ryosuke Niwa 2012-04-23 08:37:37 PDT
In my opinion, we should avoid implementing these hacks and just treat replaced contents of replaced elements as shadow content. The fallback content would work elegantly as the light DOM content without much work if we did that.
Comment 46 Dominic Mazzoni 2012-04-23 08:43:45 PDT
Ryosuke, wouldn't putting the fallback content into the canvas's shadow dom cause events to be re-targeted to the canvas? Or is there a way to avoid this?
Comment 47 Ryosuke Niwa 2012-04-23 08:55:31 PDT
(In reply to comment #46)
> Ryosuke, wouldn't putting the fallback content into the canvas's shadow dom cause events to be re-targeted to the canvas? Or is there a way to avoid this?

What I'm suggesting is to move the regular canvas content to the shadow DOM and keep the original fallback content in the light DOM (i.e. non-shadow DOM).
Comment 48 Hajime Morrita 2012-04-23 15:48:01 PDT
(In reply to comment #47)
> What I'm suggesting is to move the regular canvas content to the shadow DOM and keep the original fallback content in the light DOM (i.e. non-shadow DOM).

Maybe I missed the point but...

In this context, just using shadow dom won't help.
That is because we need to render the canvas content, and we shouldn't render the fallback content.
Shadow DOM doesn't make any non-rendered content focusable.

Maybe turning <canvas> into a shadow-dom backed element helps keeping code clean.
But we could say that's the right approach only after we have some concrete idea on
how the shadow tree of <canvas> is formed. I don'see how it will look like.

----
My 2c:

In my understanding, even RenderReplaced subclasses can have child renderers
when they override some methods. RenderMedia, a subclass of RenderImage, is one of such classes.
So if the reason to switch to RenderBlock subclass is for having render children, 
we might keep the change smaller by following RenderMedia-like approach.

It could be yet another rabbit hole though.
Comment 49 WebKit Review Bot 2012-04-23 19:33:23 PDT
Comment on attachment 138323 [details]
Patch

Attachment 138323 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12528118

New failing tests:
fast/canvas/canvas-incremental-repaint-2.html
fast/canvas/shadow-offset-7.html
compositing/webgl/webgl-nonpremultiplied-blend.html
fast/canvas/canvas-text-alignment.html
fast/canvas/image-object-in-canvas.html
batterystatus/event-after-navigation.html
accessibility/aria-disabled.html
fast/canvas/canvas-transforms-during-path.html
fast/canvas/drawImage-with-globalAlpha.html
fast/canvas/image-pattern-rotate.html
fast/canvas/fillrect-gradient-zero-stops.html
fast/canvas/quadraticCurveTo.xml
fast/canvas/toDataURL-alpha.html
compositing/webgl/webgl-background-color.html
fast/canvas/shadow-offset-5.html
fast/canvas/webgl/canvas-test.html
fast/canvas/canvas-composite.html
fast/canvas/canvas-resize-after-paint-without-layout.html
fast/canvas/canvas-incremental-repaint.html
compositing/webgl/webgl-reflection.html
fast/canvas/canvas-transform-skewed.html
fast/canvas/shadow-offset-6.html
fast/canvas/canvas-composite-transformclip.html
fast/canvas/shadow-offset-4.html
fast/canvas/canvas-composite-fill-repaint.html
fast/canvas/patternfill-repeat.html
fast/canvas/canvas-text-baseline.html
accessibility/canvas.html
fast/canvas/arc360.html
Comment 50 WebKit Review Bot 2012-04-23 19:33:29 PDT
Created attachment 138489 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 51 Dominic Mazzoni 2012-04-27 01:19:00 PDT
Hajime: the earlier patch from 2011 actually took an approach more similar to RenderMedia - it just takes RenderHTMLCanvas and extends it to allow child elements. It led to some subtle bugs and several people said they didn't like that design. I'm not sure why it makes more sense for RenderMedia. What are the children of RenderMedia?
Comment 52 Dominic Mazzoni 2012-04-30 08:44:09 PDT
Maciej, one of your comments is that there's no need to have renderers. I tried hacking on the accessibility code a bit to see what some of the implications would be of having no renderers while still exposing canvas fallback content:

1. There'd be no CSS-generated content, including not only ::before and ::after, but even things like list markers.

2. A textarea wouldn't get wrapped onto multiple lines so it'd appear like one long line.

3. There'd be no style calculations within the fallback content, so we wouldn't know to hide elements that are supposed to be display:none.

We could find ways to work around #1 or #2, but it's #3 that bothers me. How much of a hack would it be to compute the style for a node without a renderer, just to determine a few key properties like display and visibility?

Does it seem better to go down this route and try to sort out these issues as special cases, or does this make you think that it'd actually be simpler to just create renderers for the fallback content?
Comment 53 Dominic Mazzoni 2012-05-10 12:14:45 PDT
Created attachment 141221 [details]
Patch
Comment 54 chris fleizach 2012-05-10 12:37:44 PDT
Comment on attachment 141221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141221&action=review

looking pretty good to me

> Source/WebCore/accessibility/AccessibilityMenuList.cpp:43
> +    obj->init();

why do we need to make these changes?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:194
> +AccessibilityRole AccessibilityNodeObject::determineAccessibilityRole()

determineAXRole should account for ARIA roles as well. it will probably be quite common in canvi to use ARIA
Comment 55 Dominic Mazzoni 2012-05-10 15:52:38 PDT
(In reply to comment #54)
> > Source/WebCore/accessibility/AccessibilityMenuList.cpp:43
> > +    obj->init();
> 
> why do we need to make these changes?

Otherwise AccessibilityNodeObject's constructor would need to call determineAccessibilityRole, but in C++ when a constructor calls a virtual method it won't call a derived implementation of that method.

> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:194
> > +AccessibilityRole AccessibilityNodeObject::determineAccessibilityRole()
> 
> determineAXRole should account for ARIA roles as well. it will probably be quite common in canvi to use ARIA

Sure, I'll add this. Note that either way this is incomplete; we'll need to eventually make it support almost all of the roles and the rest of the methods.

- Dominic
Comment 56 James Robinson 2012-05-10 16:57:04 PDT
Comment on attachment 141221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141221&action=review

isFocusable() is pretty hot - it's called in Element::attach() for instance. I don't think we can afford an O(N) tree walk in here for a very uncommon case

> Source/WebCore/accessibility/AccessibilityNodeObject.h:84
> +    Node* m_node;

this is just a raw pointer, so who manages destroying this when the Node goes away?

> Source/WebCore/dom/Node.cpp:930
> +        while (e) {

this is a really hot function to add a really slow walk to
Comment 57 Dominic Mazzoni 2012-05-10 17:03:26 PDT
> isFocusable() is pretty hot - it's called in Element::attach() for instance. I don't think we can afford an O(N) tree walk in here for a very uncommon case

Good point. Do you think it'd be possible to add an inCanvas bit that could be set when attaching that only needs to propagate once? Other ideas?
Comment 58 James Robinson 2012-05-10 17:21:13 PDT
Do we have room for a bit on Node?

This is going to be exceedingly uncommon, maybe we could have a bit on Document or something like that saying "does anybody in this Document have canvas fallback content?" and check that before the walk.
Comment 59 Dimitri Glazkov (Google) 2012-05-10 21:53:08 PDT
Comment on attachment 141221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141221&action=review

>> Source/WebCore/accessibility/AccessibilityNodeObject.h:84
>> +    Node* m_node;
> 
> this is just a raw pointer, so who manages destroying this when the Node goes away?

This is important. In the case of RenderObject, the axObjectCache()->remove is called in willBeDestroyed. There doesn't seem anything like this for Node.
Comment 60 chris fleizach 2012-05-10 22:20:26 PDT
(In reply to comment #59)
> (From update of attachment 141221 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141221&action=review
> 
> >> Source/WebCore/accessibility/AccessibilityNodeObject.h:84
> >> +    Node* m_node;
> > 
> > this is just a raw pointer, so who manages destroying this when the Node goes away?
> 
> This is important. In the case of RenderObject, the axObjectCache()->remove is called in willBeDestroyed. There doesn't seem anything like this for Node.

FYI, there is something existing in AXObjectCache that cleans up weak Node pointers. That mechanism can be used for this as well.
Comment 61 Dominic Mazzoni 2012-05-11 00:09:48 PDT
Thanks for the feedback.

Adding a flag to Document that keeps track of whether or not there are any Canvas elements with fallback content seems like a good solution.

You're right, I need to detach (not delete) an AccessibilityNodeObject when the Node it references is deleted.

There are many other places in WebCore where ab AXObjectCache method is called to update information based on a renderer; many of these would need to be changed to update using the node instead. I wasn't planning to tackle that in this same change - but I should definitely handle remove().
Comment 62 WebKit Review Bot 2012-05-11 03:32:26 PDT
Comment on attachment 141221 [details]
Patch

Attachment 141221 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12663648

New failing tests:
accessibility/canvas-fallback-content.html
Comment 63 WebKit Review Bot 2012-05-11 03:32:33 PDT
Created attachment 141371 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 64 Dominic Mazzoni 2012-05-29 02:03:23 PDT
Created attachment 144482 [details]
Patch
Comment 65 Dominic Mazzoni 2012-05-29 02:12:05 PDT
This latest patch:
* Now detaches a AccessibilityNodeObject when the node it references is detached. Adds a new layout test to make sure this is working correctly.
* Adds a flag to Document so that Node::isFocused doesn't bother to check to see if it's a descendant of a canvas unless there's a canvas with an element child somewhere in the document.
* Moves the ARIA role calculation from AccessibilityRenderObject to AccessibilityNodeObject.
Comment 66 Early Warning System Bot 2012-05-29 03:03:19 PDT
Comment on attachment 144482 [details]
Patch

Attachment 144482 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12840520
Comment 67 Early Warning System Bot 2012-05-29 03:11:10 PDT
Comment on attachment 144482 [details]
Patch

Attachment 144482 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12843441
Comment 68 WebKit Review Bot 2012-05-29 09:42:58 PDT
Comment on attachment 144482 [details]
Patch

Attachment 144482 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12852478

New failing tests:
accessibility/canvas-fallback-content.html
accessibility/accessibility-node-memory-management.html
Comment 69 WebKit Review Bot 2012-05-29 09:43:05 PDT
Created attachment 144571 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 70 James Robinson 2012-05-29 17:17:31 PDT
Comment on attachment 144482 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144482&action=review

> Source/WebCore/dom/Node.cpp:1362
> +    Document* doc = document();
> +    if (AXObjectCache::accessibilityEnabled() && doc && doc->axObjectCacheExists())
> +        doc->axObjectCache()->remove(this);

I'm confused - Nodes in a canvas' shadow will never be attach()ed since they have no renderers, so how does this logic take care of cleanup for them?
Comment 71 Dominic Mazzoni 2012-05-30 02:21:24 PDT
Created attachment 144762 [details]
Patch
Comment 72 Dominic Mazzoni 2012-05-30 02:26:03 PDT
(In reply to comment #70)
> (From update of attachment 144482 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144482&action=review
> 
> > Source/WebCore/dom/Node.cpp:1362
> > +    Document* doc = document();
> > +    if (AXObjectCache::accessibilityEnabled() && doc && doc->axObjectCacheExists())
> > +        doc->axObjectCache()->remove(this);
> 
> I'm confused - Nodes in a canvas' shadow will never be attach()ed since they have no renderers, so how does this logic take care of cleanup for them?

I did this because detach() is called even if a node doesn't have a renderer, e.g. in this case it's called from ContainerNode::removeBetween - and I was having a hard time actually deleting a node from a layout test.

Agreed, though, that handling it in the destructor is better. I moved it there and figured out how to test it.
Comment 73 Early Warning System Bot 2012-05-30 03:10:43 PDT
Comment on attachment 144762 [details]
Patch

Attachment 144762 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12842837
Comment 74 Early Warning System Bot 2012-05-30 03:33:52 PDT
Comment on attachment 144762 [details]
Patch

Attachment 144762 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12844824
Comment 75 Gyuyoung Kim 2012-05-30 04:13:35 PDT
Comment on attachment 144762 [details]
Patch

Attachment 144762 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12847834
Comment 76 WebKit Review Bot 2012-05-30 05:29:55 PDT
Comment on attachment 144762 [details]
Patch

Attachment 144762 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12842859

New failing tests:
accessibility/canvas-fallback-content.html
accessibility/accessibility-node-memory-management.html
Comment 77 WebKit Review Bot 2012-05-30 05:30:03 PDT
Created attachment 144787 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 78 Darin Adler 2012-05-30 12:39:52 PDT
Comment on attachment 144762 [details]
Patch

Is there any way to make this *massive* patch something we can do in smaller pieces? This is just too big to be done all at once, and seems like it’s something we could easily do in smaller steps. For example, one patch just to add the “init” function, one patch to do the AccessibilityNodeObject refeactor.
Comment 79 Dominic Mazzoni 2012-05-30 13:25:07 PDT
(In reply to comment #78)
> (From update of attachment 144762 [details])
> Is there any way to make this *massive* patch something we can do in smaller pieces? This is just too big to be done all at once, and seems like it’s something we could easily do in smaller steps. For example, one patch just to add the “init” function, one patch to do the AccessibilityNodeObject refeactor.

Yes, I'd be happy to. Is this looking okay at a high level? If so, I think I can prepare the first small change now.

How about in this order:

1. Make canvas fallback content focusable, with no accessibility changes.
2. Accessibility refactoring only, no changes to behavior.
3. Final change that ties things together.
Comment 80 Dominic Mazzoni 2012-05-30 14:34:54 PDT
(In reply to comment #79)
> (In reply to comment #78)
> > (From update of attachment 144762 [details] [details])
> > Is there any way to make this *massive* patch something we can do in smaller pieces?

I'm renaming and keeping this as the "final" bug, moving the first step to:

https://bugs.webkit.org/show_bug.cgi?id=87898

Please look there for the first proposed smaller patch, thanks.
Comment 81 James Craig 2013-11-19 11:55:27 PST
Duping out to 124592, since it's unlikely this patch will be revisited.

*** This bug has been marked as a duplicate of bug 124592 ***