Bug 9589 – WCSS: -wap-accesskey not supported

• Component: CSS
• Status: RESOLVED WONTFIX

[Nicholas Shanks]

The WCSS property -wap-accesskey, equivalent to the accesskey attribute on an anchor element, is not supported in WebKit.

[Dave Hyatt]

Not relevant on the desktop, but could be relevant if/when Nokia merges to TOT.

[yichao.yin]

Created attachment 27309 [details]
initial patch for WCSS acceeskey support


WAP CSS is the companion of XHTML Mobile Profile (XHTML MP), both of them are defined in WAP 2.0 specification. This patch depends on the patch of XHMLMP, which is traced by Bug 23452.

[yichao.yin]

Created attachment 27641 [details]
updated patch contains code changes


split up the initial patch into two parts: one contains code changes, the other is for layout test.

[yichao.yin]

Created attachment 27642 [details]
patch for layout tests

[yichao.yin]

Created attachment 30324 [details]
Updated patch for adding tests

[yichao.yin]

Created attachment 30325 [details]
Updated patch for code changes

[yichao.yin]

Created attachment 30445 [details]
Latest Updated patch for code changes

Add copyright messages for code changes

[George Staikos]

Comment on attachment 30324 [details]
Updated patch for adding tests

Okay, but need to add to Qt skipped list on checkin also.

[Eric Seidel (no email)]

Comment on attachment 30324 [details]
Updated patch for adding tests

These look like manual tets, they do not belong in LayoutTests/

Can't we make these automated using DumpRenderTree?

[Eric Seidel (no email)]

Comment on attachment 30445 [details]
Latest Updated patch for code changes

Why this:
#if ENABLE(WCSS)
 1047     mutable HashMap<String, Element*, CaseFoldingHash> m_elementsByAccessKey;
 1048 #else

In general this patch looks fine.  non-harmful.  The testing looks inadequate however, see my previous comment.

I can't really approve this until there are better tests. :(

[yichao.yin]

(In reply to comment #10)
> (From update of attachment 30445 [details] [review])
> Why this:
> #if ENABLE(WCSS)
>  1047     mutable HashMap<String, Element*, CaseFoldingHash>
> m_elementsByAccessKey;
>  1048 #else

Here using String instead of original StringImpl* to keep the hash key is because the hash keys 
are generated dynamically for WCSS acceesskey.


> In general this patch looks fine.  non-harmful.  The testing looks inadequate
> however, see my previous comment.
> I can't really approve this until there are better tests. :(

Okay, I will consider making the tests better.

[Charles Wei]

Created attachment 33151 [details]
This is for -wap-accesskey support , contains both the code change and the test cases as per request from last review 

This patch contains both the code change and the test cases /expected test results.  There's no change to the last code change,  this patch differes from last patch only in the test cases, test cases for LayoutTests has been designed and expected test results are also attached.

[Eric Seidel (no email)]

Comment on attachment 33151 [details]
This is for -wap-accesskey support , contains both the code change and the test cases as per request from last review 

We need more information in the ChangeLog in order to make a productive review.  What is this change doing, why is it doing it.  What are the changes to the files?

[George Staikos]

(In reply to comment #10)
> (From update of attachment 30445 [details])
> Why this:
> #if ENABLE(WCSS)
>  1047     mutable HashMap<String, Element*, CaseFoldingHash>
> m_elementsByAccessKey;
>  1048 #else
> 
> In general this patch looks fine.  non-harmful.  The testing looks inadequate
> however, see my previous comment.
> 
> I can't really approve this until there are better tests. :(

(In reply to comment #13)
> (From update of attachment 33151 [details])
> We need more information in the ChangeLog in order to make a productive review.
>  What is this change doing, why is it doing it.  What are the changes to the
> files?

Forgetting your previous review? :)  I think it's quite clear what it does, and it seems that you felt so before also.

[Charles Wei]

Created attachment 34197 [details]
resubmit the patch with a more descriptive ChangeLog following the review comments    

I am now resubmitting the patch with a more descriptive ChangeLog following Eric's comments.

[Eric Seidel (no email)]

Comment on attachment 34197 [details]
resubmit the patch with a more descriptive ChangeLog following the review comments    

Why does WebKit want this?

This seems like a non-critical feature of an obscure spec.  This if for mobile no?  I don't know of a mobile device with an alt key, but I believe you there is one.  How many pages on the WAP web actually depend on this parsing?  Is there a CSS3 equivilant that we can just map this property to?

You change to the ChangeLog to tell me to read the spec, isn't very helpful.  Reviewing is often hard.  There are 80 patches in the queue right now.  Patches which are easy to r+ are the ones which get reviewed.  In this case, it's not obvious why this is a good thing for the project, why it's correct, or even what it does.  There is a relatively large amount of code to review in this patch (2-3 pages of code).

I'm not trying to be difficult. But if you want this reviewed, you need to make it easy for the reviewers to r+ it.  Much of that work is on selling your patch.  making it small, making the ChangeLog and tests super-clear.  Patches which aren't easy to r+ just sit in the queue for forever, until someone like me comes along and r-'s them.

Also, I don't think you want to make me the requestee.  :) That's likely only to slow down your reviews as others will ignore them.

Please add more useful information to the ChangeLog, including answering some of the questions I posed above.

[George Staikos]

This was already discussed on webkit-dev.  Also I should point out that Google's Android team seems to be very interested according to my source, so I think there is interest from your own organization too. As previously pointed out, WAP gateways make use of XHTML-MP and associated features, and we'd like to have full mobile support.  It's legacy, but you can't just make the world disappear simply because you don't like the protocol it talks.  We've been maintaining it for more than a year now so I think that's enough evidence that it will be maintained.

[Eric Seidel (no email)]

What device would use this?  It still seems like an irrelevant portion of the spec.

[Eric Seidel (no email)]

Comment on attachment 34197 [details]
resubmit the patch with a more descriptive ChangeLog following the review comments    

Changes like these should be explained in the ChangeLog or in the source:
 #if ENABLE(WCSS)
 1068     mutable HashMap<String, Element*, CaseFoldingHash> m_elementsByAccessKey;
 1069 #else
10671070     mutable HashMap<StringImpl*, Element*, CaseFoldingHash> m_elementsByAccessKey;
 1071 #endif

I don't see how this is changed from the last patch I r-d.  I didn't just r- this because I disagree with WAP. ;)  I r- this because it's difficult to r+ without more information as to what's going on here.

If we're adding a parser, why don't we have parser tests for this?

[Charles Wei]

This article explains advantages and disadvantatges of accesskey attribute of HTML 4,  and some proposals about how the webpage author should design their page to take advantage of the access key.
 
http://www.cs.tut.fi/~jkorpela/forms/accesskey.html


I did some more test of accesskey attribute of HTML4 on different browser , and here's the findings:

On IE6/7/8,  Alt+accesskey, and then Enter to activiate the element (link, button,etc)
On Firefox,  Alt+Shift+accesskey to activate the element 
On Safari on Windows,  Alt + accesskey to activate the element
On Opera 9, Shift+Esc will list all the accesskeys , and then press corresponding accesskey to activate the element it associates with 
Chrome:  doesn't support accesskey .

[Charles Wei]

Created attachment 34971 [details]
patch that fixes the comments by the reviewer

This patch is for WCSS -wap-accesskey support. 

-wap-accesskey is an extension to CSS2 by OMA, to assign an accesskey for elements, for easy navigation purpose. pretty much like accesskey attribute of HTML.

We are limiting the valid accesskey to number 0 ~ 9, # and *, since they are available on all keypad devices.

This patch differences from the last patch -- 34197 : 

1. moved the property name to the WCSS-specific .in file.
2. fixed some bug in parsing.
3. re-designed the accesskey invocation algorithm
4. new test cases designed

[Eric Seidel (no email)]

How does a wap-enabled device (phone) use these?  Are there wap-enabled devices with wcc -wap-accesskey support on the market today?  Are there pages which use this?

[George Staikos]

(In reply to comment #22)
> How does a wap-enabled device (phone) use these?  Are there wap-enabled devices
> with wcc -wap-accesskey support on the market today?  Are there pages which use
> this?

There are many millions of such devices out there, for sure.  They all behave differently because it depends on the physical hardware and how it can be mapped to the browser.  It's primarily for non-touchscreen phones where getting to a link is not always easy.

[Charles Wei]

(In reply to comment #22)
> How does a wap-enabled device (phone) use these?  Are there wap-enabled devices
> with wcc -wap-accesskey support on the market today?  Are there pages which use
> this?


OMA is playing a very important role in the wireless field,  and most of the operators will require the mobile device to be OMA standard compliant. XHTML-MP, ESMP, WCSS are the important Web browsers from OMA , which are derived from the internet standard, XHTML, ECMA and CSS respectively. We need to make WebKit to support this standards to promote it to Wireless devices.

[Charles Wei]

Anybody who can help to review this ?   any comments ?

[Eric Seidel (no email)]

Comment on attachment 34971 [details]
patch that fixes the comments by the reviewer

+Element* Document::getElementByAccessKey(const String& key) const

seems like a really slow way to crawl through the DOM for the access keys.  Why can't one expand the m_elementsByAccessKey strategy?  querySelector would answer the CSS side of the question.

style:
+        if (accessKeys.contains(key)) {
+            return element;
+        }

Spelling:
+        //2, Try to get accecckeys from internal WCSS and external WCSS in turn

spacing:
+        String accessKeys ; //accesskeysForElement(element, m_styleSelector);

Honestly I don't really want this change.  I still don't understand why this obscure part of a dying spec is needed, and clearly no other reviewer seems to care enough to review it either since this bug has been ignored for over 7 months in the queue.

r- for the nits above.

[Charles Wei]

Created attachment 38919 [details]
new patch address reviewer's comments

This patch addresses the following comments by the reviewer:
1. style issue
2. spelling issue
3. Spacing issue 
4. use m_elementsByAccessKey strategy to store the [accesskey, element] pair in m_elementsByAccessKey , this is done only once   for a document, next time you have a accesskey pressed,  it will try to get the element from m_elementsByAccessKey directly instead tranversing the DOM.
   To do that , m_elementsByAccessKey was changed 
  from :
    mutable HashMap<StringImpl*, Element*, CaseFoldingHash> m_elementsByAccessKey;
  to: 
    mutalbe HashMap<String, Element*, CaseFoldingHash> m_elementsByAccessKey;

   because we can't hold a StringImpl* since the accesskey in the style sheet, while before the change,  the index of m_elementsByAccessKey is of type "StringImpl*", which is hold by an element in it's "accesskey" attribute .

5.  As to the mention of "querySelector would answer the CSS side of the question" , I don't know how to apply that to this feature.  basically what we would like to do is to go through all elements and find those with "-wap-accesskey" style or with "accesskey" attribute,  if an element with both "accesskey" attribute and "-wap-accesskey" style applied, the -wap-accesskey takes precedence .

To address the reviewer's other comments: 

 "why this obscure part of a dying spec is needed",  actually WCSS is an emergying spec instead of a dying spec, which is initiatied by OMA, an big player in Mobile industry, for Mobile devides. OMA standard compliance is a requirement of must for many operators and device manufactures,  implementation of WCSS and other OMA specs in WebKit definitely promotes WebKit into a wider spectrum of devices.

[George Staikos]

Comment on attachment 38919 [details]
new patch address reviewer's comments

The function in Document seems a bit inefficient.  Is there any way to do this better?

Other than that I think the patch is fine and has addressed all the comments.

[George Staikos]

*** Bug 29360 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Given that no one has reviewed this in 2 months, and I have since decided that I am against adding this feature to WebKit, I think we should reject this patch/bug due to lack of reviewer support.

[Eric Seidel (no email)]

Comment on attachment 38919 [details]
new patch address reviewer's comments

Rejecting patch due to lack of reviewer support, per my comment from 2 weeks ago.

[George Staikos]

Niko can you help review these?  Otherwise I"ll take care of it int he next week.

[Adam Barth]

style-queue ran check-webkit-style on attachment 38919 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 38919 [details]
new patch address reviewer's comments

Still lacking reviewer support again 6 weeks later, this is just wasting space in the review queue.  WebKit's apathy seems to be serving as rejection of this feature.  I suggest we close this bug as WONTFIX.

[Alexey Proskuryakov]

Bug 23477 was resolved with a general lack of desire to implement any WCSS extensions, so closing this one, too. Please e-mail webkit-dev mailing list if you are interested in WCSS support.