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

Try editor heading #3801

Merged
merged 4 commits into from
Dec 11, 2017
Merged

Try editor heading #3801

merged 4 commits into from
Dec 11, 2017

Conversation

jasmussen
Copy link
Contributor

This adds a page heading for screen readers. If this mitigates #503, then we'd want to make sure that the correct post type is output, and the text is translatable.

This adds a page heading for screen readers. If this mitigates #503, then we'd want to make sure that the correct post type is output, and the text is translatable.
@jasmussen jasmussen added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Dec 4, 2017
@jasmussen jasmussen self-assigned this Dec 4, 2017
@jasmussen jasmussen added the [Status] In Progress Tracking issues with work in progress label Dec 4, 2017
@afercia
Copy link
Contributor

afercia commented Dec 5, 2017

It's an improvement: at least, it would help screen reader users. I still think this main h1 should be visible though, to give a clear context to everyone about what they're editing.
Currently, there's no indication if I'm editing a post, a page, or maybe a CPT testimonial, a project, a product...

Also, at this point the post title shouldn't be a h1 wrapping a textarea. We're in an editing context and the main topic of the page is "Edit post", not the post title. I've heard there were plans to change the post title in a block, not sure about the progress there.

@jasmussen
Copy link
Contributor Author

jasmussen commented Dec 6, 2017

There are certainly two aspects to #503, one is the screen reader label, the other I would suggest is a design consideration. These two can absolutely be the same, and commonly are, but I would like to explain why such a page title has not been part of the design yet. As you read that, it doesn't mean it can't be — we've already gone through many iterations of the editor bar as well as the design, so it's not off the table.

In any case, the reason I did not include that title, is that we have the following interface to indicate visually what you'r looking at:

screen shot 2017-12-06 at 09 07 44

The purpose here is to free up as much space as possible for the editing canvas, and minimizing UI at the same time.

How would you feel about trying this approach for now (given corrections so the actual post type is correctly output and the whole thing is translatable)? As we continue to improve the screen reader experience and testing the UI, we'd then be able to better decide whether an explicit page title is necessary or not (and if it is, we can easily reuse the code).

An alternate approach is to show the CPT in the save status indicator:

screen shot 2017-12-06 at 09 19 56

So "Saved Page" instead of "Saved", etc. This would be in addition to the <h1>.

Thoughts?

@jasmussen
Copy link
Contributor Author

Oh also, can you elaborate on the following?

Also, at this point the post title shouldn't be a h1 wrapping a textarea. We're in an editing context and the main topic of the page is "Edit post", not the post title. I've heard there were plans to change the post title in a block, not sure about the progress there.

What would mitigate this? Should the post title not be a h1?

@afercia
Copy link
Contributor

afercia commented Dec 6, 2017

The purpose here is to free up as much space as possible for the editing canvas, and minimizing UI at the same time.

I understand what the purpose is 🙂 I guess the point is exactly this: we're omitting a functional feature that's useful for all users, not just screen readere users, for the sake of visual "cleanliness".

While the admin menu gives some indication when it's opened, I'd like to remind it can be collapsed based on user settings and it's completely hidden in the responsive view:

screen shot 2017-12-06 at 09 50 23

screen shot 2017-12-06 at 09 50 50

In these cases there's really no indication of what is being edited.

I'd certainly go for the visually hidden h1 for now, as it is an improvement. I'd recommend to keep the door open to a visible heading though. Worth considering this hi doesn't necessarily need to look like "big text": it can be styled.

As per the post title being a h1: a web page should have just one h1, and it should indicate what the page is about. We're in an editing context here, this is not the frontend. So when users land on the edit post page, the question the h1 should answer is:

  • what this page is about?
  • answer: this page is about editing a post (or page etc.)

Instead, the post title answer this question only in the frontend, where it is actually the main topic fo the page. In the backend, it doesn't. Think of a post with a title "Hello world". When you're editing in the backend, would "Hello world" identify the topic/purpose of the page? Certainly not, as the main topic / purpose of the page is about editing a post so the main h1 should say "Edit post" or "Edit post Hello World".

Also, from a semantic perspective, in an editing context the post title doesn't need to be a heading in the first place.

@jasmussen
Copy link
Contributor Author

Great thoughts, thanks for that.

Instead, the post title answer this question only in the frontend, where it is actually the main topic fo the page. In the backend, it doesn't. Think of a post with a title "Hello world". When you're editing in the backend, would "Hello world" identify the topic/purpose of the page? Certainly not, as the main topic / purpose of the page is about editing a post so the main h1 should say "Edit post" or "Edit post Hello World".

Also, from a semantic perspective, in an editing context the post title doesn't need to be a heading in the first place.

So you're saying that one solution could be that the h1 could be the screen reader text (visible or otherwise), and the post-title could be a h2 in the editing context and output a h1 in the frontend context? Just want to make sure I read you right.

@afercia
Copy link
Contributor

afercia commented Dec 6, 2017

the h1 could be the screen reader text (visible or otherwise)

yes

and the post-title could be a h2 in the editing context and output a h1 in the frontend context?

no: in the editing context it shouldn't be a heading at all. Instead, it should be an editable field (textarea or input or editable if it will be changed in a block)

@jasmussen
Copy link
Contributor Author

I pushed a change to make the post-title component use a textarea wrapped in a div, instead of a textarea wrapped in a h1. Does that address the 2nd item?

@afercia
Copy link
Contributor

afercia commented Dec 6, 2017

Looks good to me! (maybe the div can be removed entirely?)

@afercia
Copy link
Contributor

afercia commented Dec 6, 2017

Worth noting the other headings level should be reviewed, as at the moment there's a skipped level:

headings

Please let me know if you want me to open a new issue.

@jasmussen
Copy link
Contributor Author

(maybe the div can be removed entirely?)

I tried, but we can't style the textarea alone to sufficiently look like a block, and given it might one day be a block, seemed like it would be worth to keep a wrapping element.

Worth noting the other headings level should be reviewed, as at the moment there's a skipped level

I can make those h2's, I guess that would help?

@afercia
Copy link
Contributor

afercia commented Dec 6, 2017

I can make those h2's, I guess that would help?

That would be great.

This better matches the page hierarchy.
@jasmussen
Copy link
Contributor Author

Changed the sidebar headings. Does this fix #2024?

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Code changes look good to me. I tested this and I did not found any regression or design changes. If this change addresses the a11y issues it is good to go.

@afercia
Copy link
Contributor

afercia commented Dec 8, 2017

@jasmussen thanks for working on this! I've realized that also other headings need to be bumped up by one level. For example, expanding "Categories & Tags" reveals two h4 headings that should be now changed in h3 😬
Not sure if there are other headings in the sidebar that would need the same treatment.

@jasmussen
Copy link
Contributor Author

Good catch. I think I fixed it above, but I also went through a lot of the block settings which were H3s and should presumably H2s, so changed that.

I appreciate the previous review, @jorgefilipecosta, but would appreciate a second pair of eyes before I merge as that last commit was sllliiiightly big. Thanks so much.

@jasmussen jasmussen removed the [Status] In Progress Tracking issues with work in progress label Dec 8, 2017
Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

Code changes look good to me.

@jasmussen jasmussen merged commit 67f6402 into master Dec 11, 2017
@jasmussen jasmussen deleted the try/edit-heading branch December 11, 2017 08:04
@jasmussen jasmussen restored the try/edit-heading branch December 11, 2017 08:30
@jasmussen
Copy link
Contributor Author

Merged this a bit too quickly, so reverted. Going to revisit and make sure "Edit Post" is also both translatable and contextual (i.e. Edit Page, Edit [CPT]) etc.

jasmussen pushed a commit that referenced this pull request Dec 12, 2017
This is round 2 of #3801.

It mitigates/fixes #503 and fixes #2024.

What remains to be done in this branch is:

- Make sure the text is translatable, right now it's hardcoded to "Edit Post"
- Make sure the text is contextual (so it shows Edit Page, Edit CPT etc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants