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

Fixes http://github.com/SAP/openui5/issues/207 #209

Closed
wants to merge 29 commits into
base: master
from

Conversation

Projects
None yet
@RalphMarshall
Contributor

RalphMarshall commented Oct 31, 2014

Added the ariaDescribedBy and ariaLabelledBy associations to the
sap.m.Button control. Also updated the explored page for buttons to use
the new associations. I added a new disabled button to the explored sample
to ensure that bringing over the code from the desktop version of Button,
which used a slightly different handling of the disabled property, still
worked correctly.

In addition to the functional changes I made some minor changes to eliminate problems flagged by your ESLint process. Since your contribution guidelines include a clean run of ESLint as a prerequisite I figured this was the right thing to do, but if I should ignore such warnings in existing code please let me know.

Note that I already have a signed corporate contributor agreement on file with SAP.

sirion and others added some commits Oct 24, 2014

[FIX] layout/Splitter: Correctly calculate content sizes
After an orientation change the Splitter bars were incorrectly sized
if the Splitter was not rerendered. This fix changes the order of
calculation and resets the Splitter bars before using them to calculate
the available height/width.

Change-Id: I17a84c6799f60f191b33de1291a8b9cfe4eb7bd3
[FIX] layout/Splitter: Correctly align bar handle icons
CSS: 0120031469 0000815329 2014
Change-Id: I9d7a503c51d22f3102fff23a16b8185c1886644c
[FIX] Correctly detect Chrome on iOS
CSS: 0120061532 0001361328 2014
Change-Id: I26cd1be69cd0db18c0a035f2fe8247bbbb71bc85
Fixes http://github.com/SAP/openui5/issues/207
Added the ariaDescribedBy and ariaLabelledBy associations to the
sap.m.Button control. Also updated the explored page for buttons to use
the new associations. I added a new disabled button to the explored sample
to ensure that bringing over the code from the desktop version of Button,
which used a slightly different handling of the disabled property, still
worked correctly.
@Michadelic

This comment has been minimized.

Show comment
Hide comment
@Michadelic

Michadelic Nov 3, 2014

Contributor

The change looks very good to me. You even fixed a couple of code issues in the button. Thanks for also taking care of all the JSDoc comments, this greatly improves our code quality.

We just recently introduced more ESLint checks, and not all files are adapted to the new code quality standards. That is why you probably hit a couple of ESLint problems when making the fix.

Will wait for a second opinion before we merge this one, but fine from my side.

Contributor

Michadelic commented Nov 3, 2014

The change looks very good to me. You even fixed a couple of code issues in the button. Thanks for also taking care of all the JSDoc comments, this greatly improves our code quality.

We just recently introduced more ESLint checks, and not all files are adapted to the new code quality standards. That is why you probably hit a couple of ESLint problems when making the fix.

Will wait for a second opinion before we merge this one, but fine from my side.

@matz3

This comment has been minimized.

Show comment
Hide comment
@matz3

matz3 Nov 3, 2014

Contributor

The indention is not correct in some lines (spaces instead of tabs). Apart from that, this also looks good to me, thanks @RalphMarshall !

Contributor

matz3 commented Nov 3, 2014

The indention is not correct in some lines (spaces instead of tabs). Apart from that, this also looks good to me, thanks @RalphMarshall !

[INTERNAL] sap.ui.core.format.NumberFormat: improve documentation of
RoundingMode

Change-Id: I7d3f0ced78d40be254565d1d6e3a8a6fa2dab047
@RalphMarshall

This comment has been minimized.

Show comment
Hide comment
@RalphMarshall

RalphMarshall Nov 3, 2014

Contributor

Thanks for all of the detailed feedback, and I'm glad it sounds like the code will work for everybody. Is there anything else I need to do with this, or is it ready to pull it into the master branch when you get around to it?

Contributor

RalphMarshall commented Nov 3, 2014

Thanks for all of the detailed feedback, and I'm glad it sounds like the code will work for everybody. Is there anything else I need to do with this, or is it ready to pull it into the master branch when you get around to it?

@thomaskoetter

This comment has been minimized.

Show comment
Hide comment
@thomaskoetter

thomaskoetter Nov 3, 2014

Contributor

You don't currently need to do anything else. However, I've requested internally that your pull request not be merged yet. Not for technical reasons, but because we have some people discussing the overall screen reader concept for the sap.m library. I just want to make sure that your change is in line with what else we want to implement. Please, give us another week or two until we get back to you.

Contributor

thomaskoetter commented Nov 3, 2014

You don't currently need to do anything else. However, I've requested internally that your pull request not be merged yet. Not for technical reasons, but because we have some people discussing the overall screen reader concept for the sap.m library. I just want to make sure that your change is in line with what else we want to implement. Please, give us another week or two until we get back to you.

@RalphMarshall

This comment has been minimized.

Show comment
Hide comment
@RalphMarshall

RalphMarshall Nov 5, 2014

Contributor

Can you say more about your discussions regarding screen reader support? Thanks!

Contributor

RalphMarshall commented Nov 5, 2014

Can you say more about your discussions regarding screen reader support? Thanks!

@akudev

This comment has been minimized.

Show comment
Hide comment
@akudev

akudev Nov 5, 2014

Contributor

There were questions whether we want to maintain the names "ariaDescribedBy" etc. and whether we'd like to change anything else about the approach before doing the sap.m screenreader support work, but I think they are resolved now and there is only the normal code review pending now.
Sorry for the delay, this is just initial hiccup. :-)

Contributor

akudev commented Nov 5, 2014

There were questions whether we want to maintain the names "ariaDescribedBy" etc. and whether we'd like to change anything else about the approach before doing the sap.m screenreader support work, but I think they are resolved now and there is only the normal code review pending now.
Sorry for the delay, this is just initial hiccup. :-)

@akudev

This comment has been minimized.

Show comment
Hide comment
@akudev

akudev Nov 5, 2014

Contributor

Hi Ralph,

the main responsible developer is currently very busy with other topics, so I'll jump in for the code review.
In general it would be good to sync in advance which controls will be addressed when, so we can reserve some time on our side and avoid delays. Thomas will try to get in touch with you. It can then still take some days, but don't get irritated in this case, people are really busy here.

Regarding the change: mostly good and loads of welcome cleanup, thanks!
There are three minor things that need to be done and 2-3 questions:

  1. In addition to the corporate license agreement, you also need to state your personal consent with the Individual Contributor License Agreement (sorry, that's what our lawyers told us). Just add the respective sentence here as comment, or in the commit message, see https://github.com/SAP/openui5/blob/master/CONTRIBUTING.md#contributor-license-agreement for details.
  2. The commit message has the correct "Fixes..." statement, but the first line should describe what it actually does. Like:
    [FEATURE] sap.m.Button: add ARIA/screenreader support
    This is the line that will appear in the release notes, hence a short explanation is important. See https://github.com/SAP/openui5/blob/master/docs/guidelines.md#git-guidelines for a bigger example (you don't need to write that much stuff, and the Change-Id will be added by us).
  3. The indentation should be done with tabs, not spaces.
  4. The only questions regarding the code are around tabindex/disabled:
    a. why did you remove the "disabled" attribute?
    b. why do you add a tabindex=0? Isn't a button anyway focusable?
    c. tabindex=-1 for disabled state is to make the Button focusable by clicking? But would that be a use-case for users of screenreaders or is information also used by screenreaders?
    (sorry, it has been 2 years or so since I last did ARIA implementation...)

Once these points are done/clarified, the change could be merged.

Again sorry for the bumpy start, there was initially some misunderstanding: it was not clear that this is the start of the SAS contributions, plus the control development team is anyway getting to the ARIA stuff soon.
You don't need to open issue tickets, by the way, just creating pull requests is fine.

Thanks and regards
Andreas

Contributor

akudev commented Nov 5, 2014

Hi Ralph,

the main responsible developer is currently very busy with other topics, so I'll jump in for the code review.
In general it would be good to sync in advance which controls will be addressed when, so we can reserve some time on our side and avoid delays. Thomas will try to get in touch with you. It can then still take some days, but don't get irritated in this case, people are really busy here.

Regarding the change: mostly good and loads of welcome cleanup, thanks!
There are three minor things that need to be done and 2-3 questions:

  1. In addition to the corporate license agreement, you also need to state your personal consent with the Individual Contributor License Agreement (sorry, that's what our lawyers told us). Just add the respective sentence here as comment, or in the commit message, see https://github.com/SAP/openui5/blob/master/CONTRIBUTING.md#contributor-license-agreement for details.
  2. The commit message has the correct "Fixes..." statement, but the first line should describe what it actually does. Like:
    [FEATURE] sap.m.Button: add ARIA/screenreader support
    This is the line that will appear in the release notes, hence a short explanation is important. See https://github.com/SAP/openui5/blob/master/docs/guidelines.md#git-guidelines for a bigger example (you don't need to write that much stuff, and the Change-Id will be added by us).
  3. The indentation should be done with tabs, not spaces.
  4. The only questions regarding the code are around tabindex/disabled:
    a. why did you remove the "disabled" attribute?
    b. why do you add a tabindex=0? Isn't a button anyway focusable?
    c. tabindex=-1 for disabled state is to make the Button focusable by clicking? But would that be a use-case for users of screenreaders or is information also used by screenreaders?
    (sorry, it has been 2 years or so since I last did ARIA implementation...)

Once these points are done/clarified, the change could be merged.

Again sorry for the bumpy start, there was initially some misunderstanding: it was not clear that this is the start of the SAS contributions, plus the control development team is anyway getting to the ARIA stuff soon.
You don't need to open issue tickets, by the way, just creating pull requests is fine.

Thanks and regards
Andreas

Wenqian Wang and others added some commits Nov 5, 2014

Wenqian Wang
[FEATURE] sap.m.Token - token select/deselect
- create ontap function to select/deselect token
- qunit test

Change-Id: Ic6e5a79e3a5cd99bcf1323020a0ef561d26f2197
[INTERNAL][FIX] sap.m.ComboBoxBase: HCB theme clean up
Change-Id: Icf0d929597c0eb2c0d929f43020c845d2d851c6c
[FIX] v2.ODataModel: change default of refreshAfterChange to true
Change-Id: Ibc7a04ff43b6829da092021dced75b0c53e7e5bb
[FEATURE] OPA: Page Objects + Opa.resetConfig()
Provide the capability of page objects in OPA, including an example.
This should be the new way to foster reuse of arrangements, actions and
assertions.

Allowing to reset the Opa.config to its default.

Change-Id: I3374d8a99bf130f369e5c3e50026e741e5a1fd84
[INTERNAL] sap.m.Shell: Added styles for HCB
Change-Id: I820fb58b12d2700f7ca018aeab0d2f92fe6639aa
[INTERNAL][FIX] sap.m.ObjectHeader: KH Outline and HCB Optimization
- KH add additional outline on active element
- HCB theme optimization

Change-Id: I086ed08aa5a82d747956b9056545c11336665fd6
[FIX] sap.m.TablePersoDialog: Change DOM structure
[INTERNAL]
-Remove the List control from the select all toolbar.
-Put select all checkbox and reset button directly
 in the toolbar.
-Add a function to get the id of the select all
 checkbox.
-Change the model header to be an object instead
 of an array.
-Add stylesheet into the base theme so that the label
 from the checkbox matches the one from the list items
 and to position the reset button at the right.
-Convert base/library line ending to LF.
-Add qunit test for keyboard handling.
-Fix bug when calculating the scroll containere height.
-Remove scroll container from tab chain in FF.

Change-Id: Ic931ec8214969776b699a09bb75b97d66a336ab2
[INTERNAL] Remove white spaces in combobox.qunit.html
Change-Id: I382d6ee6a85c640e0b44ac55519e8ef3b6fd8327
@RalphMarshall

This comment has been minimized.

Show comment
Hide comment
@RalphMarshall

RalphMarshall Nov 6, 2014

Contributor

I am working on making adjustments as requested above. First, the legal portion: I hereby declare to agree to the OpenUI5 Individual Contributor License Agreement.

Contributor

RalphMarshall commented Nov 6, 2014

I am working on making adjustments as requested above. First, the legal portion: I hereby declare to agree to the OpenUI5 Individual Contributor License Agreement.

@RalphMarshall

This comment has been minimized.

Show comment
Hide comment
@RalphMarshall

RalphMarshall Nov 6, 2014

Contributor

The only questions regarding the code are around tabindex/disabled:
a. why did you remove the "disabled" attribute?
b. why do you add a tabindex=0? Isn't a button anyway focusable?
c. tabindex=-1 for disabled state is to make the Button focusable by clicking? But would that be a use-case for users of screenreaders or is information also used by screenreaders?

My answers:

I didn't actually remove the disabled attribute - I just changed where it is written out. Now that I'm using the writeAccessibilityState() method I passed the enabled flag in as an option to it.

Tabindex=0 is probably overkill, but it also doesn't hurt, and I thought this was a little clearer. On the other hand, tabindex=-1 says that the button is not in the tab order, so keyboard users will not get to it as they move around using the tab key. Screen-readers will generally go to such elements, announcing them as disabled when appropriate, as they don't necessarily use the tab order. (I think that JAWS does have a mode that uses standard keyboard navigation rather than a special cursor, and in that case the button will be skipped).

Contributor

RalphMarshall commented Nov 6, 2014

The only questions regarding the code are around tabindex/disabled:
a. why did you remove the "disabled" attribute?
b. why do you add a tabindex=0? Isn't a button anyway focusable?
c. tabindex=-1 for disabled state is to make the Button focusable by clicking? But would that be a use-case for users of screenreaders or is information also used by screenreaders?

My answers:

I didn't actually remove the disabled attribute - I just changed where it is written out. Now that I'm using the writeAccessibilityState() method I passed the enabled flag in as an option to it.

Tabindex=0 is probably overkill, but it also doesn't hurt, and I thought this was a little clearer. On the other hand, tabindex=-1 says that the button is not in the tab order, so keyboard users will not get to it as they move around using the tab key. Screen-readers will generally go to such elements, announcing them as disabled when appropriate, as they don't necessarily use the tab order. (I think that JAWS does have a mode that uses standard keyboard navigation rather than a special cursor, and in that case the button will be skipped).

RalphMarshall added some commits Oct 31, 2014

[FEATURE] sap.m.Button: add ARIA/screenreader support
Added the ariaDescribedBy and ariaLabelledBy associations to the
sap.m.Button control. Also updated the explored page for buttons to use
the new associations. I added a new disabled button to the explored sample
to ensure that bringing over the code from the desktop version of Button,
which used a slightly different handling of the disabled property, still
worked correctly.

Fixed indentation and other whitespace

Standardized all JSDoc to use @return instead of @returns

Fixes #207
@RalphMarshall

This comment has been minimized.

Show comment
Hide comment
@RalphMarshall

RalphMarshall Nov 6, 2014

Contributor

I have pushed the requested changes to my branch. Note that when I merged my forked version with the SAP/master and then did the push I show all of the other commits that took place between my original commit and this latest one. I figure I need to bring in all of the other changes before committing, but if there is a cleaner way to do this I'm open to suggestions.

Contributor

RalphMarshall commented Nov 6, 2014

I have pushed the requested changes to my branch. Note that when I merged my forked version with the SAP/master and then did the push I show all of the other commits that took place between my original commit and this latest one. I figure I need to bring in all of the other changes before committing, but if there is a cleaner way to do this I'm open to suggestions.

@akudev

This comment has been minimized.

Show comment
Hide comment
@akudev

akudev Nov 6, 2014

Contributor

Hi Ralph,

well, yeah, I know what tabindex=-1 does :-), but when 'disabled="disabled"' is set, the button is anyway not in the tab chain. Only because you removed 'disabled="disabled"', the tabindex=-1 becomes necessary.

You say that you oly moved writing the "disabled" attribute. Actually I don't think writeAccessibilityState() writes this attribute. It DOES write 'aria-disabled="true"', but that means nothing to browsers and at least I don't see the 'disabled="disabled"' in my tests of your code.
Maybe you can check on your side; if you can confirm that the "disabled" attribute is no longer there, I'd like to ask you to re-add it.

Your change also removes the exclamation mark of the initial copyright comment template in Button.js. Is there a reason for that?

I'm also seeing some whitespace changes that confuse me, I'll have another look and come back to you later.

Just as a remark: for private methods we usually do not require JSDoc. Documentation where the method is not trivial is good, but formal JSDoc documentation of "oRenderManager" and "oButton" in tiny methods in the ButtonRenderer is... maybe too much. At least we don't have this 100% documentation policy for private methods.

The other commits that slipped in look messy in GitHub, but fine when I import your changes, so that's fine for the moment. Rebasing your change before pushing would probably help(?).

Thanks a lot and regards
Andreas

Contributor

akudev commented Nov 6, 2014

Hi Ralph,

well, yeah, I know what tabindex=-1 does :-), but when 'disabled="disabled"' is set, the button is anyway not in the tab chain. Only because you removed 'disabled="disabled"', the tabindex=-1 becomes necessary.

You say that you oly moved writing the "disabled" attribute. Actually I don't think writeAccessibilityState() writes this attribute. It DOES write 'aria-disabled="true"', but that means nothing to browsers and at least I don't see the 'disabled="disabled"' in my tests of your code.
Maybe you can check on your side; if you can confirm that the "disabled" attribute is no longer there, I'd like to ask you to re-add it.

Your change also removes the exclamation mark of the initial copyright comment template in Button.js. Is there a reason for that?

I'm also seeing some whitespace changes that confuse me, I'll have another look and come back to you later.

Just as a remark: for private methods we usually do not require JSDoc. Documentation where the method is not trivial is good, but formal JSDoc documentation of "oRenderManager" and "oButton" in tiny methods in the ButtonRenderer is... maybe too much. At least we don't have this 100% documentation policy for private methods.

The other commits that slipped in look messy in GitHub, but fine when I import your changes, so that's fine for the moment. Rebasing your change before pushing would probably help(?).

Thanks a lot and regards
Andreas

@RalphMarshall

This comment has been minimized.

Show comment
Hide comment
@RalphMarshall

RalphMarshall Nov 6, 2014

Contributor

Andreas,

Thanks again for taking the time. You were completely correct about the disabled/aria-disabled mistake - I'm not sure how I overlooked it in my own testing. In any case, I think I have it corrected now. The removal of the exclamation mark in the header was a mistake. I originally thought that I'd introduced it where it wasn't supposed to be, so I was cleaning up after myself. I now see that they are in all of the headers, so I know to leave them be.

As for the JSDoc on private methods - I agree with your point, as we also do not normally document them either. However, they had existing JSDoc-style comments on them, which caused the eslint checks to complain about them. I considered editing the comment style to be a plain comment as an alternative, but wasn't sure what to do.

Contributor

RalphMarshall commented Nov 6, 2014

Andreas,

Thanks again for taking the time. You were completely correct about the disabled/aria-disabled mistake - I'm not sure how I overlooked it in my own testing. In any case, I think I have it corrected now. The removal of the exclamation mark in the header was a mistake. I originally thought that I'd introduced it where it wasn't supposed to be, so I was cleaning up after myself. I now see that they are in all of the headers, so I know to leave them be.

As for the JSDoc on private methods - I agree with your point, as we also do not normally document them either. However, they had existing JSDoc-style comments on them, which caused the eslint checks to complain about them. I considered editing the comment style to be a plain comment as an alternative, but wasn't sure what to do.

@akudev

This comment has been minimized.

Show comment
Hide comment
@akudev

akudev Nov 7, 2014

Contributor

Change is merged, closed by 81ba90c

(sorry, I didn't notice that the "fixes" statement in the commit message only referred to the issue ticket. "Closes #209" would also have closed this pull request automatically)

So thanks a lot for the first SAS contribution!
This will be in version 1.26 and in any 1.25.* preview releases we may publish until then.

Future patches would be easier to handle on our side if the additional changes after a code review are done inside the original commit (with "amend commit"). And of course we should get rid of those other unrelated commits in the pull requests. I still think a rebase would have helped, but we managed without. ;-)

Contributor

akudev commented Nov 7, 2014

Change is merged, closed by 81ba90c

(sorry, I didn't notice that the "fixes" statement in the commit message only referred to the issue ticket. "Closes #209" would also have closed this pull request automatically)

So thanks a lot for the first SAS contribution!
This will be in version 1.26 and in any 1.25.* preview releases we may publish until then.

Future patches would be easier to handle on our side if the additional changes after a code review are done inside the original commit (with "amend commit"). And of course we should get rid of those other unrelated commits in the pull requests. I still think a rebase would have helped, but we managed without. ;-)

@akudev akudev closed this Nov 7, 2014

@RalphMarshall

This comment has been minimized.

Show comment
Hide comment
@RalphMarshall

RalphMarshall Nov 7, 2014

Contributor

Thanks for taking time to work through this with us. I'll be talking to people here to see what we can do to streamline the process for future submissions.

Contributor

RalphMarshall commented Nov 7, 2014

Thanks for taking time to work through this with us. I'll be talking to people here to see what we can do to streamline the process for future submissions.

@akudev

This comment has been minimized.

Show comment
Hide comment
@akudev

akudev Nov 7, 2014

Contributor

Yeah, we are learning on both sides... actually "amend" will probably not be possible once you pushed, but squashing several commits into one should work, see http://davidwalsh.name/squash-commits-git
Another contribution (SAP/grunt-openui5#1) has been done this way.

Contributor

akudev commented Nov 7, 2014

Yeah, we are learning on both sides... actually "amend" will probably not be possible once you pushed, but squashing several commits into one should work, see http://davidwalsh.name/squash-commits-git
Another contribution (SAP/grunt-openui5#1) has been done this way.

@thomaskoetter

This comment has been minimized.

Show comment
Hide comment
@thomaskoetter

thomaskoetter Nov 7, 2014

Contributor

@RalphMarshall, it would be great if we could align before your next submission. Please, contact me at firstname.lastname @ sap.com.

Regards,
Thomas

Contributor

thomaskoetter commented Nov 7, 2014

@RalphMarshall, it would be great if we could align before your next submission. Please, contact me at firstname.lastname @ sap.com.

Regards,
Thomas

@akudev akudev added the fixed label Jun 24, 2016

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