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

In custom labels mozilla #7194

Closed

Conversation

ManshulBelani
Copy link
Contributor

@ManshulBelani ManshulBelani commented May 23, 2017

Link to issue number:

#5874

Summary of the issue:

Sometimes web elements are not properly labelled for the screen reader to read. This fix helps the user to add customised labels to such web elements once and then every time the user accesses the page, this custom label will be spoken when focus is on the particular element.

Description of how this pull request fixes the issue:

This pull request implements the solution for mozilla and Chrome (Canary version). For Internet Explorer refer branch in_t5874_new
It currently supports links, images and form elements like edit boxes,combo boxes, buttons, multi line edit areas, radio buttons and check boxes.
Regarding extending the functionality to Edge we would start working on it once this gets released.

The changes made in this commit are saving the object in focus at the time of when addLabel function is called and sending this object as an argument to the function. The virtual buffer object is also sent to the function as an argument.
After adding the custom label to config file, the virtual buffer is unloaded (vBuffer.unloadBuffer()), the buffer is then reloaded again (vBuffer.loadBuffer()) so that it is in turn created again with the custom labels added to the buffer.

Testing performed:

some basic level testing performed. Working for links graphics and form fields.

Known issues with pull request:

Right now after adding the custom labels, the user has to refresh the web page once before NVDA starts reading them.

Change log entry:

The section and description of this change to use for the changes file. Valid sections are:

  • New features
  • Changes
  • Bug fixes

For examples see the changes file in your NVDA repo: user_docs/en/changes.t2t or on github: https://github.com/nvaccess/nvda/blob/master/user_docs/en/changes.t2t

@ManshulBelani
Copy link
Contributor Author

This pull request has code for adding custom labels to web elements in IE, mozilla and Chrome(Canary version).

@ManshulBelani
Copy link
Contributor Author

The branch allows adding custom labels in Internet Explorer, Mozilla Firefox and Chrome Canary.
Auto refresh of buffer after adding custom label has also been handled now.

@bhavyashah
Copy link

bhavyashah commented Nov 13, 2017 via email

@ManshulBelani
Copy link
Contributor Author

It currently supports links, images and form elements like edit boxes,combo boxes, buttons, multi line edit areas, radio buttons and check boxes.
Regarding extending the functionality to Edge we would start working on it once this gets released.

@ManshulBelani
Copy link
Contributor Author

@michaelDCurran: I have added the auto refresh of buffer functionality after adding custom label to the code. Kindly review the code and provide feedback.
Would it be possible to add this fix in the next NVDA build?

@feerrenrut
Copy link
Contributor

Please can you describe how the changes made address the problem? An explanation of how this issue fixes the problem will greatly speed up its review and increase our confidence in the change. This should include a broad level description of the design decisions made for this change.

When you are happy with this, please "request a review" by using the github reviewers menu.

@ManshulBelani
Copy link
Contributor Author

The changes made in this commit are saving the object in focus at the time of when addLabel function is called and sending this object as an argument to the function. The virtual buffer object is also sent to the function as an argument.
After adding the custom label to config file, the virtual buffer is unloaded (vBuffer.unloadBuffer()), the buffer is then reloaded again (vBuffer.loadBuffer()) so that it is inturn created again with the custom labels added to the buffer.

@feerrenrut
Copy link
Contributor

I have added this information to the description. However, at this point I don't believe this is an adequate technical description of the changes. This is a large change, that requires some non-trivial design decisions. Understanding the rationale for these decisions and the alternatives considered would make the process of reviewing the code more straightforward, and will save time in the long term maintenance of this code, as well as providing a roadmap for how the original author expects this code to be improved over time.

One example, is that the labels are persisted by storing the information in (config) files. There has been no discussion or explanation of why this is preferable over another approach. One concern here is how this scales over time? How many labels causes a degradation in performance.

While many of these questions can be answered by looking at the code, it is slower to do so. Looking at the code does not give any insight into the thought process behind the approach.

@ManshulBelani
Copy link
Contributor Author

@feerrenrut : Please find attached the design document for the solution.
Custom Labels Design Overview.docx

@dineshkaushal
Copy link
Contributor

dineshkaushal commented Jan 18, 2018 via email

@feerrenrut
Copy link
Contributor

I think there needs to be more discussion of the overall approach taken, so that the caveats are known. Here is my interpretation of the current PR, and the concerns I have:

  • Labels are saved in a folder 'webLabels' under the NVDA settings folder.
    • For each page with labels a file is created, named with the URL of the site you visit with '.ini' appended.
    • Some special characters are removed from the filename, this is not done using a library function and no indication of given the set of characters removed is safe.
    • How does this handle URL's that change as you use the page? Or URL's that are updated from time to time?
    • When will these files get cleaned up?
  • There is not really a clear discussion of how keys for label are generated, and how robust this is.
  • This relies on Virtual buffer support which is unlikely to be available in the future. Some discussion on how this could be implemented without virtual buffers, or will this feature have to be removed?

@Adriani90
Copy link
Collaborator

@ManshulBelani any update on this? Did you find some time to address @feerrenrut's comments? Thanks for your work.

@feerrenrut
Copy link
Contributor

We understand this has been a desired feature. This PR requires a bit of work before we can accept it and isn't currently a priority at NV Access. Anyone who wishes to take this on may, but please consider the following:

  • Conflicts and updated to Python 3 code.
  • Consider new applications now available, eg Edge
  • Further documentation on the PR about how it works and what cases have been considered, why certain things were done or decisions made.

For now we will close this PR.

@feerrenrut feerrenrut closed this Apr 1, 2020
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

6 participants