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

Add "Image Title Attribute" as an editable attribute on the image block #11070

Merged
merged 4 commits into from
Nov 1, 2019

Conversation

brentswisher
Copy link
Contributor

@brentswisher brentswisher commented Oct 25, 2018

Description

Closes #11054.

Adds a title attribute to the image block, as requestions in #11054, although I think there may be some more discussion needed, I will follow up there.

How has this been tested?

Add a new image block to the page either via dropping a file of the media library
Open the settings for that block.
See that there is now a "title" attribute that is defaulted to the current title attribute in the media library
Save and view post, see that the title attribute exists on the image unless it was set to blank.

Screenshots

edit_post_ gutenberg_dev _wordpress

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@Soean Soean mentioned this pull request Jan 14, 2019
@gziolo gziolo added [Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement. labels Feb 1, 2019
@gziolo gziolo requested a review from a team February 1, 2019 09:36
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 1, 2019
@youknowriad youknowriad added Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility labels Feb 1, 2019
@jasmussen
Copy link
Contributor

This seems okay to me. We might want to put the Title text below the Image size tools, as those seem more important, but I can understand how there's an argument to keep it close to the Alt text too. This, and the fact that the sidebar is getting crowded, would be the only design considerations we need to think about. Once that's settled, it'd be nice with shorter copy for the help text — how short can we make it?

I would suggest the acessibility feedback is more important. I know that in some cases, the Title attribute can be detrimental, but would appreciate thoughts from others.

@youknowriad
Copy link
Contributor

Seeing all the help texts makes me wonder if we shouldn't leverage a ⓘ icon for them. I feel the usability of the UI is starting to suffer from all these texts.

@jasmussen
Copy link
Contributor

Seeing all the help texts makes me wonder if we shouldn't leverage a ⓘ icon for them. I feel the usability of the UI is starting to suffer from all these texts.

I personally am of the opinion that we should work to retire that icon entirely. I think an in between step is to merge it with the block traversal button so at least those two are one — but the more buttons we can remove from the left hand side of the top toolbar, IMO, the better.

I feel like the sidebar is the right place for it. You can toggle the sidebar off for a simple basic experience, and anything you put in the sidebar should be stuff that is advanced. The tools we have at our disposal to simplify here is to reduce text as much as is possible, simplify labels as much as possible, only add help text when it's actually necessary, and group items in panels.

@youknowriad
Copy link
Contributor

@jasmussen What I meant is that instead of having help text display under each input, we'd display a ⓘ icon or similar next to the label.

@youknowriad
Copy link
Contributor

Definitely something out of scope of this PR though.

@jasmussen
Copy link
Contributor

Doh, sorry I thought you meant the Outline button in the top toolbar.

I don't know that adding an icon with help text would reduce that much more — but I think we can go a long way in simply reducing labels and help texts to as concise text as possible. Of course the meaning has to be conveyed, but nearly always there's a shorter phrasing.

@brentswisher
Copy link
Contributor Author

Thanks for the feedback @jasmussen and @youknowriad. I'll bring this code up to date so it can be reviewed/merged, in the meantime, any thoughts on the concern I brought up about the default titles in my comment:

2. Since the default title is set to the file name, I am concerned that adding it here will result in a lot more titles being saved into the page's content with the default (ie: IMG_2323),

Specifically, I'm wondering about images "inheriting" defaulted titles that match their filename.
I know on my sites, that would mean a lot of images get title attributes like "IMG0024.jpg" without me really doing anything.

The only alternative I could think of would be a toggle for if the title should be displayed that is defaulted to off? Then at least the user could see the title they are adding and it's more opt-in than opt-out. The obvious trade-off being adding complexity to an already full UI.

@youknowriad
Copy link
Contributor

@brentswisher I'm having trouble understanding (by reading the code) where this default title is being added. I'm assuming by default it should be undefined.

@brentswisher
Copy link
Contributor Author

0hmwpswfej

Sorry, should have made that more clear. When an image is uploaded, the media uploader sets the title attribute of that image to be the file name. This is an existing procedure for every image, so when I grab the image attributes in the block, that comes in as the title.

This is the line:
const imageProps = pick( image, [ 'alt', 'title', 'id', 'link', 'caption' ] );

Maybe that's fine and it's obvious enough that is a user wants to change it they will. My worry is that the majority of users might not notice it or understand what is does and we will populate the internet with pointless title attributes.

I'm open to leaving it as is if others think it's okay, just wanted to at least make sure it was a conscious decision.

@youknowriad
Copy link
Contributor

What if we remove the "title" from this pick call? What downsides will we create? Why this "title" property was there for the first time?

@brentswisher
Copy link
Contributor Author

I was thinking the problem would be that two different attributes for an image both called title seemed confusing. However, in looking at the classic editor it looks like this is exactly what's happening. There is a title for the media item which serves as its name. There is also an "image title attribute" that adds a title to the actual HTML output. I say we follow that example, I'll work on updating.

@mapk
Copy link
Contributor

mapk commented Feb 6, 2019

To help the help text 😉 , maybe we can change it to say something like this:

"Provide a clear name for this image."

screen shot 2019-02-05 at 7 20 21 pm

I'm also concerned that the title's default being the image name could cause some weirdness as pointed out by @brentswisher above.

@brentswisher
Copy link
Contributor Author

I worked on this some more and not defaulting the title was quite confusing to me. There were two things called title for an image. One which was edited in the media modal and one which was in the block, but were unrelated to each other. I expected them to act the same as the alt attribute when uploading or replacing an image.

What about a toggle? This way we can keep it with only one title, but make sure it is intended to be displayed?

Something like this (Copy/design is rough, just a proof of concept)

hpdnhb00dc

@brentswisher brentswisher force-pushed the add/image-block-title-attribute branch from 7f614ed to d709b31 Compare September 11, 2019 16:02
@brentswisher
Copy link
Contributor Author

I've updated the help text to be a link as suggested by @afercia and the text to "Provide an optional description for this image." @mapk "optional, non-essential" seemed a little repetitive without adding any additional information, but if there are strong feelings it should be that I'm not opposed to updating. Love to get this wrapped up and merged if everyone is good with it?

@afercia
Copy link
Contributor

afercia commented Sep 11, 2019

@brentswisher thanks for the ping! Maybe it's a language thing (not an native English speaker here) but as a user, if the whole sentence is linked:

[link]Provide an optional description for this image.[/link]

Screenshot 2019-09-11 at 19 27 59

I would expect the link to point to a resource that explains what an optional description is. Instead, we'd want the message to inform users they shouldn't rely on the title attribute for important information.

Today, title attributes are only available to a minority of users. I'd tend to think we can contribute to the progress of the Web as a whole if we strive a bit to find some better wording :)

Also, the part: description for this image is not 100% accurate because this is about the purpose of the image, not necessarily the image content (same as for the alt attribute).

What about something along the lines of:

Optional description. [link]Consider to not rely on the title attribute for important information.[/link]

(I'm trying to avoid don't and negative sentences)

/Cc @mapk

@brentswisher
Copy link
Contributor Author

@afercia Yes, I like it not all being a link better, I reworded the second part a little bit:
Edit_Post_‹_WordPress_Develop_—_WordPress_1

@mapk mapk added the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Sep 17, 2019
@michelleweber
Copy link

I find it a little odd to only include language about how not to use this field, and no language about what the field actually is. Am thinking of something along the lines of:

Describe the role of this image on the page. (Note: many devices and browsers do not display this text.)

This explains what it is, and by explaining that it's not always used the "don't put key info here" idea is strongly implied.

@afercia
Copy link
Contributor

afercia commented Sep 18, 2019

I'd tend to agree, see my previous comment where I was trying to avoid negative sentences.

@brentswisher
Copy link
Contributor Author

Happy to update that, would it all be the link text, or just a part of it @michelleweber?

@mapk
Copy link
Contributor

mapk commented Sep 27, 2019

Let's try @michelleweber's idea there. It's worded pretty good!

@michelleweber
Copy link

I'd only link the second sentence.

@brentswisher brentswisher force-pushed the add/image-block-title-attribute branch from c17b985 to b52615f Compare September 27, 2019 16:10
@brentswisher
Copy link
Contributor Author

Updated to the latest text:
Edit_Post_‹WordPress_Develop—_WordPress

@mapk
Copy link
Contributor

mapk commented Oct 1, 2019

One more suggestion from:

Note: many devices and browsers do not display this text.

to:

Note: this text is not always displayed.

or simpler:

Not always displayed.

I think the link defines the reasons about devices, etc.

@michelleweber
Copy link

My two cents -- whenever possible, text should make sense and answer people's questions as much as possible without requiring them to click on a link. In this case, "Not always displayed" doesn't answer questions, it raises them -- why? when? where? -- and that requires clicking. (You could lighten the line visually by only linking "do not display" instead of the whole sentence, tho.)

(I know the longer version doesn't answer every question either, but it does clarify that if its not displayed, it's because of the browser or device and not because your website/something you did.)

@mapk
Copy link
Contributor

mapk commented Oct 3, 2019

In this case, "Not always displayed" doesn't answer questions, it raises them -- why? when? where? -- and that requires clicking.

This is fair, but for what it's worth, "Many devices and browsers do not display this text." probably raises the same questions too.

I'm good either way here. I'd love it if there was a way to trim it down, but not opposed to longer text if it helps the user.

@brentswisher brentswisher force-pushed the add/image-block-title-attribute branch from 1781266 to 7c8d9c3 Compare October 16, 2019 00:36
@brentswisher
Copy link
Contributor Author

This has been held up for quite a while with differing opinions on text, but nobody seems strongly opposed to what is there now. Am I reading that correctly? If so feel free to speak up, otherwise, I am going to try to get this moving forward as is.

@brentswisher
Copy link
Contributor Author

This PR has been going on for a while so a summary may be in order:

  • This PR adds a field for "Image Title Attribute" to the Advanced options of the image block.
  • This is different from the "Attachment Title" which is never displayed publicly.
  • This lines up with how the classic editor handles the title attribute (which is currently unavailable in Gutenberg)
  • While adding this to add compatibility to the classic editor, help text and a link to the w3c page explaining that it should not be used for critical information was added as title has historically been abused for accessibility purposes.

@brentswisher brentswisher changed the title Add title as an editable attribute on the image block Add "Image Title Attribute" as an editable attribute on the image block Oct 30, 2019
@mkaz
Copy link
Member

mkaz commented Oct 30, 2019

The code change looks good, however I think the extra text and link is confusing. Especially since we have almost the same text above in the Alt Text field above it. It is hard as a user to parse and figure out what I'm supposed to do. See this screenshot.

Screenshot from 2019-10-30 06-43-25

This is what the fields look like in the Media Uploader, and there is no extra contextual info around title there.

Screenshot from 2019-10-30 06-55-27

I thinking remove the extra text around the Title field will make it easier for the user, also gets the feature in so the users can set the title which is an accessibility win. We can solve the contextual help by addressing @youknowriad comment about using a (i) icon in a separate isuse.

@mkaz mkaz merged commit 680a085 into WordPress:master Nov 1, 2019
daniloercoli added a commit that referenced this pull request Nov 5, 2019
…rnmobile/gb-mobile-872-JSApplicationIllegalArgumentException-in-RCTAztecView

* 'master' of https://github.com/WordPress/gutenberg: (56 commits)
  Update: Default gradients. (#18214)
  Fix: setting a preset color on pullquote default style makes the block invalid (#18194)
  Fix default featured image size (#15844)
  Fix postmeta radio regression. (#18183)
  Components: Switch screen-reader-text to VisuallyHidden (#18165)
  [rnmobile] Release 1.16.0 to master (#18261)
  Template Loader: Add theme block template resolution. (#18247)
  Add a README file for storybook directory (#18245)
  Add editor-gradient-presets to get_theme_support (#17841)
  Add "Image Title Attribute" as an editable attribute on the image block (#11070)
  enables horizontal movers in social blocks (#18234)
  [RNMobile] Add mobile Spacer component (#17896)
  Add experimental `ResponsiveBlockControl` component (#16790)
  Fix mover for floats. (#18230)
  Rename Component to WPComponent in docstring (#18226)
  Colors Selector: replace `Aa` text by SVG icon (#18222)
  Removed gif from README (#18200)
  makes the submenu items stacked vertically (#18221)
  Add block navigator to sidebar panel for nav block (#18202)
  Fix: consecutive updates may trigger a blocks reset (#18219)
  ...
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Copy Review Needs review of user-facing copy (language, phrasing) Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image block: allow for editing image title