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

Fix and clean up modal dialog example #713

Closed
wants to merge 4 commits into from
Closed

Fix and clean up modal dialog example #713

wants to merge 4 commits into from

Conversation

kleinfreund
Copy link
Contributor

This pull request addresses the following issues:

Clean up HTML:

  • Fixed highlight.js not being triggered due to code markup using a <div> element instead of <pre> and <code> elements.
  • HTML head:
    • Consistent attribute order
    • Omitting unnecessary type attributes and closing slashes
  • Content:
    • When used inside code, replaced <q> elements with regular double quote characters (i.e. ")
    • Added manual line breaks to the example code so that the example code block doesn’t have a scrollbar on medium to large screens
  • Added missing </li> closing elements in some places
  • Added a bunch of empty lines between some chunks of code for easier readability
  • Fixed some minor indentation issues in some places

Clean up JavaScript:

  • Fixed some documentation comments using JSDoc
  • Removed the ability to pass an ID to the focusAfterClosed parameter because it isn’t used at all. This added unnecessary complexity to the example.
  • Removed the ability to pass an element node to the focusFirst parameter. Same reason as above.

Fixed Layout Issue:

Removed the div#base_window_layer element wrapping the main content that was used to disable scrolling when any modal dialog is open.

The new implementation disables scrolling on the root element as soon as one dialog is open and restores the original property as soon as all dialoges are closed.

This fixed a major visual discrepancy of the dialog example page layout compared to the appearance of all other example pages. Most notably, the main content wasn’t aligned to the left of the screen and the main scrollbar was rendered a couple of pixels inside the viewport instead of the viewport border.

Potential bug:

The following code looks broken.

aria.Utils.attemptFocus:

try {
  element.focus();
}
catch (e) {
}
aria.Utils.IgnoreUtilFocusChanges = false;

Is the last line supposed to be called inside the catch block? If not, what’s the purpose of the try-catch block if nothing happens?

@mcking65
Copy link
Contributor

@kleinfreund, thank you, but we are getting close to merging #688 by @sh0ji for issue #101, which includes at least one of the same changes while adding the alert dialog example for APG 1.1 release 2. I suggest that after we merge #688 that you rebase and resubmit this.

@kleinfreund
Copy link
Contributor Author

Ah, yikes, I did look at that pull request but disregarded it due to its title and first couple of commit messages. Will have a look into it once that pull request is merged.

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.

2 participants