Skip to content
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

Custom labels for MSHTML #6619

Closed
wants to merge 15 commits into from
Closed

Conversation

ManshulBelani
Copy link
Contributor

@ManshulBelani ManshulBelani commented Dec 6, 2016

Assigning and accessing custom labels to web page components (form fields, links and graphics)

Issue #5874

@feerrenrut
Copy link
Contributor

@jcsteh It looks like this one would be best handled by you. See issue #5874

Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from all other comments here, also please fix up your braces so that they conform to the same c++ coding style already in the files. I.e. opening brace at the end of the if statement line, closing and opening braces on same line as the else etc.

@@ -48,7 +48,7 @@ interface VBuf {
* @param backendName The name of the backend (the path to the correct dll will be calculated automatically)
* @return a handle identifying the new virtual buffer.
*/
VBufRemote_bufferHandle_t createBuffer([in] handle_t bindingHandle, [in] int docHandle, [in] int ID, [in,string] const wchar_t* backendName);
VBufRemote_bufferHandle_t createBuffer([in] handle_t bindingHandle, [in] int docHandle, [in] int ID, [in,string] const wchar_t* backendName,[in,string] const wchar_t* labels);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the code documentation above this method to mention the new argument.

@@ -1072,6 +1075,7 @@ if(!(formatState&FORMATSTATE_INSERTED)&&nodeName.compare(L"INS")==0) {
// There is alt text, so use it.
contentString = tempIter->second;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accidentally added blank line.

}
}
if ((tempIter = attribsMap.find(L"HTMLAttrib::src")) != attribsMap.end()) {
renderChildren=true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does renderChildren have to get set to true here?

@@ -1140,7 +1163,33 @@ if(!(formatState&FORMATSTATE_INSERTED)&&nodeName.compare(L"INS")==0) {
attribsMap.find(L"HTMLAttrib::aria-label") != attribsMap.end() || attribsMap.find(L"HTMLAttrib::aria-labelledby") != attribsMap.end()
|| attribsMap.find(L"HTMLAttrib::title") != attribsMap.end() || attribsMap.find(L"HTMLAttrib::alt") != attribsMap.end()
)))
attribsMap[L"name"]=IAName;
{attribsMap[L"name"]=IAName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place the left brace ({) on the line above to conform with code formatting for the rest of the file.

@@ -1130,6 +1143,16 @@ if(!(formatState&FORMATSTATE_INSERTED)&&nodeName.compare(L"INS")==0) {
||nodeName.compare(L"MATH")==0
) {
contentString=L" ";
} else if((nodeName.compare(L"A")==0)&&(attribsMap.find(L"HTMLAttrib::href")!=attribsMap.end())) {
renderChildren=true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I don't understand why renderChildren needs to be set to true.

@@ -366,7 +374,19 @@ def loadBuffer(self):

def _loadBuffer(self):
try:
self.VBufHandle=NVDAHelper.localLib.VBuf_createBuffer(self.rootNVDAObject.appModule.helperLocalBindingHandle,self.rootDocHandle,self.rootID,unicode(self.backendName))
weblink=self.documentConstantIdentifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly move all this config fetching code into its own method which just returns the serialized labels string ready to pass to createBuffer.

for k,v in config.iteritems():
labels+=k+":"+v+","

self.VBufHandle=NVDAHelper.localLib.VBuf_createBuffer(self.rootNVDAObject.appModule.helperLocalBindingHandle,self.rootDocHandle,self.rootID,unicode(self.backendName),unicode(labels))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is only supported for MSHTML so far, it might be best to add a class viarable on VirtualBuffer which can turn on and off support for labels. This would be set to True in mshtml's VirtualBuffer but False in the base VirtualBuffer, until all VirtualBuffers finally support labels. Otherwise you're reading in files and passing needless data when it willbe never used.

@@ -358,3 +370,4 @@ def shouldPassThrough(self, obj, reason=None):
except COMError:
pass
return super(MSHTML, self).shouldPassThrough(obj, reason)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally added blank line I think.

except Exception as e:
pass

def getFilenameFromElementDomain(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps move this function into its own custom web labels module and then it can be used from the base VirtualBuffer as well... I think there is some code duplication.

@michaelDCurran michaelDCurran changed the title In t5874 new Custom labels for MSHTML Jan 17, 2017
# elif (not nameAttribute):
# customLabel=self.obj.rootNVDAObject.getCustomLabel(idAttribute)
# if customLabel:
# attrs["name"]=customLabel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commented code should be removed.

Copy link
Contributor

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelDCurran is reviewing this instead.

Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you didn't add the customLabels.py file to the repository?

labels=self.fetchLabels()
self.VBufHandle=NVDAHelper.localLib.VBuf_createBuffer(self.rootNVDAObject.appModule.helperLocalBindingHandle,self.rootDocHandle,self.rootID,unicode(self.backendName),unicode(labels))
else:
self.VBufHandle=NVDAHelper.localLib.VBuf_createBuffer(self.rootNVDAObject.appModule.helperLocalBindingHandle,self.rootDocHandle,self.rootID,unicode(self.backendName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this does in deed work (not providing labels argument) it is probably hiding an error. Better to just have one call to createBuffer, but set labels to u"" if labels are disabled.

self.VBufHandle=NVDAHelper.localLib.VBuf_createBuffer(self.rootNVDAObject.appModule.helperLocalBindingHandle,self.rootDocHandle,self.rootID,unicode(self.backendName),unicode(labels))
if (self.labelSupportOn):
labels=self.fetchLabels()
self.VBufHandle=NVDAHelper.localLib.VBuf_createBuffer(self.rootNVDAObject.appModule.helperLocalBindingHandle,self.rootDocHandle,self.rootID,unicode(self.backendName),unicode(labels))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than forcing labels to unicode here, it would be best that labels are unicode all of the way (I.e. they are unicode within the labels file itself if they are not already).

@ManshulBelani
Copy link
Contributor Author

refer to pull request #7194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants