New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support the HTML5 required attribute in MSHTML documents #7321

Merged
merged 4 commits into from Sep 5, 2017

Conversation

Projects
None yet
3 participants
@leonardder
Collaborator

leonardder commented Jun 24, 2017

Link to issue number:

As far as I know, this pr has no dedicated issue for itself. This pr is based on findings in #7320 (comment)

Summary of the issue:

NVDA does not report the required state of a html element when the HTML5 required attribute is used for this.

Description of how this pull request fixes the issue:

Added support for required in de NVDAHelper MSHTML vbuf backend and associated python code.

Testing performed:

For required

<input id="name" required type="text" />

For not required

<input id="name" type="text" />

Note that required="false" will still result in required state being added, but this is no bug according to the spec. I've seen the same behaviour in Firefox.

Change log entry:

I consider the absence of this feature to be a bug.

  • Bug fixes
    • In MSHTML documents, the HTML5 required attribute is now supported to indicate the required state of a form field (#7321)
@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Jun 28, 2017

Contributor

This looks good so far, but can you please also support this in focus mode. See aria-required in NVDAObjects\IAccessible\MSHTML.py

Contributor

michaelDCurran commented Jun 28, 2017

This looks good so far, but can you please also support this in focus mode. See aria-required in NVDAObjects\IAccessible\MSHTML.py

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jun 28, 2017

Collaborator

Hmm, good point. However, it seems this is going to be more difficult than I thought

<input type="text" required="required" name="cheese" />
>>> nav.HTMLNode.getAttribute('type')
u'text'
>>> nav.HTMLNode.getAttribute('name')
u'cheese'
>>> type(nav.HTMLNode.getAttribute('required'))
<type 'NoneType'>

It seems the node doesn't expose the required attribute correctly.

Collaborator

leonardder commented Jun 28, 2017

Hmm, good point. However, it seems this is going to be more difficult than I thought

<input type="text" required="required" name="cheese" />
>>> nav.HTMLNode.getAttribute('type')
u'text'
>>> nav.HTMLNode.getAttribute('name')
u'cheese'
>>> type(nav.HTMLNode.getAttribute('required'))
<type 'NoneType'>

It seems the node doesn't expose the required attribute correctly.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jun 28, 2017

Collaborator
Collaborator

leonardder commented Jun 28, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jun 28, 2017

Collaborator

@michaelDCurran: this is now working.

I added a contains method to the HTMLAttribCache to support stuff like 'required' in self.HTMLAttributes. I decided to give it a separate cache, as I didn't want to change the behaviour of getitem. This nieuw contains_ method fixes a bug where 'attribute' in HTMLAttributes resulted in a freeze of NVDA.

Collaborator

leonardder commented Jun 28, 2017

@michaelDCurran: this is now working.

I added a contains method to the HTMLAttribCache to support stuff like 'required' in self.HTMLAttributes. I decided to give it a separate cache, as I didn't want to change the behaviour of getitem. This nieuw contains_ method fixes a bug where 'attribute' in HTMLAttributes resulted in a freeze of NVDA.

@@ -71,6 +71,7 @@ class HTMLAttribCache(object):
def __init__(self,HTMLNode):
self.HTMLNode=HTMLNode
self.cache={}
self.containsCache={}

This comment has been minimized.

@michaelDCurran

michaelDCurran Jul 5, 2017

Contributor

This can be a set rather than a dictionary

@michaelDCurran

michaelDCurran Jul 5, 2017

Contributor

This can be a set rather than a dictionary

This comment has been minimized.

@leonardder

leonardder Jul 5, 2017

Collaborator

Mm, not sure whether we're on the same track here. Changing this to a set means that we lose track of the attributes for which hasAttribute has returned False in the past, so in other words, False evaluations won't get cached. We either need a dictionary for this, or have two sets for hasAttribute True and hasAttribute False. Could it be that you overlooked this, or is it your intention not to cache the Falses?

@leonardder

leonardder Jul 5, 2017

Collaborator

Mm, not sure whether we're on the same track here. Changing this to a set means that we lose track of the attributes for which hasAttribute has returned False in the past, so in other words, False evaluations won't get cached. We either need a dictionary for this, or have two sets for hasAttribute True and hasAttribute False. Could it be that you overlooked this, or is it your intention not to cache the Falses?

@@ -84,6 +85,23 @@ def __getitem__(self,item):
self.cache[item]=value
return value
def __contains__(self,item):
try:
return self.containsCache[item]

This comment has been minimized.

@michaelDCurran

michaelDCurran Jul 5, 2017

Contributor

Then change this to return item in self.containsCache

@michaelDCurran

michaelDCurran Jul 5, 2017

Contributor

Then change this to return item in self.containsCache

This comment has been minimized.

@leonardder

leonardder Jul 5, 2017

Collaborator

If we do this, the block below that actually runs hasAttribute never gets executed.

@leonardder

leonardder Jul 5, 2017

Collaborator

If we do this, the block below that actually runs hasAttribute never gets executed.

Show outdated Hide outdated source/NVDAObjects/IAccessible/MSHTML.py Outdated
@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Jul 5, 2017

Contributor

Make sure to test on several versions of Internet Explorer. At very least what ever Internet Explorer comes with Windows 7.

Contributor

michaelDCurran commented Jul 5, 2017

Make sure to test on several versions of Internet Explorer. At very least what ever Internet Explorer comes with Windows 7.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jul 5, 2017

Collaborator
Collaborator

leonardder commented Jul 5, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jul 18, 2017

Collaborator

@michaelDCurran: Would you be able to explain why you belief containsCache should be a set rather than a dictionary? See #7321 (comment)

If you agree with containsCache being a dictionary anyway, this is ready for another review.

Collaborator

leonardder commented Jul 18, 2017

@michaelDCurran: Would you be able to explain why you belief containsCache should be a set rather than a dictionary? See #7321 (comment)

If you agree with containsCache being a dictionary anyway, this is ready for another review.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Jul 19, 2017

Contributor

This looks good to me. Ignore the set/dictionary comment.

Contributor

michaelDCurran commented Jul 19, 2017

This looks good to me. Ignore the set/dictionary comment.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Jul 19, 2017

Contributor

@leonardder If you are finnished with this I'll incubate it?

Contributor

michaelDCurran commented Jul 19, 2017

@leonardder If you are finnished with this I'll incubate it?

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jul 19, 2017

Collaborator

@michaelDCurran commented on 19 jul. 2017 03:37 CEST:

@leonardder If you are finnished with this I'll incubate it?

Yes, go ahead. Thanks a lot!

Collaborator

leonardder commented Jul 19, 2017

@michaelDCurran commented on 19 jul. 2017 03:37 CEST:

@leonardder If you are finnished with this I'll incubate it?

Yes, go ahead. Thanks a lot!

michaelDCurran added a commit that referenced this pull request Jul 19, 2017

@michaelDCurran michaelDCurran merged commit 985144f into nvaccess:master Sep 5, 2017

@nvaccessAuto nvaccessAuto removed the incubating label Sep 5, 2017

@nvaccessAuto nvaccessAuto added this to the 2017.4 milestone Sep 5, 2017

@josephsl josephsl referenced this pull request Oct 15, 2017

Merged

Docs17.4 #7662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment