Fixes issues with placeholder state and url; adds error property. #44

bicknellr · 2015-11-20T22:27:43Z

opus1269 · 2015-11-25T03:27:27Z

I was testing this out and ran into a couple of problems:

The !! in the following binding doesn't work:

    <img id="img" role="none" hidden$="[[!!sizing]]">

I added back in the computed property to fix it:

    <img id="img" role="none" hidden$="[[_computeImageVisibility(sizing)]]">

The following CSS rules do not work:

    :host([sizing]) #backdrop {
      display: block;
    }

    :host([sizing]) #img {
      display: none;
    }

Pretty sure sizing needs to be an attribute for this to work. I added the reflectToAttribute to fix it:

      sizing: {
        type: String,
        reflectToAttribute: true,
        value: null
      },

I'm not sure if that's the right thing to do. Actually, with the hidden properties working, I'm not sure the various CSS rules changing the display attribute are even needed.

I can do a PR but I'm pretty new to this and don't know the procedure for a PR on another PR.

bicknellr · 2015-11-25T20:15:49Z

Thanks for looking this over! IE10 doesn't pay attention to hidden and we still (mostly) support it so I'd say leave the CSS for that in for now; otherwise you're right, these rules should be redundant. Although this could be cleaner if replaced with:

[hidden] {
  display: none;
}

Also, yup, looks like !! doesn't work. :/ Going back to the computed property sounds good, if it's used in both places where !sizing was before for consistency. (The original name was a bit misleading too, maybe _computeImageHidden (or similar) would be more clear as it's returning whether or not the image is hidden rather than visible.)

Making a PR to a PR is really easy: every PR is just asking to merge one branch into another, so all you have to do is make a new request to merge your branch into this one (bicknellr/state) rather than master. When you're comparing changes from your branch and are about to make a PR you'll see two dropdown menus:

Here, you can specify which branch you would like to take changes from ('compare:') and which branch you'd like to merge those changes into ('base:', defaults to master).

If you want to submit a PR with some changes, I'd be glad to take them! (Otherwise, I can address the things you've pointed out once @cdata is ready to review.)

opus1269 · 2015-11-27T03:12:07Z

One more issue:

The EncodeURI on backdropImage and placeholder will be a breaking change for many users (including me :). It will double encode an already encoded URI used as input. If there isn't a really good reason for it, I would leave it out. If not, it probably needs to be clearly documented that raw input is expected.

opus1269 · 2015-11-27T03:33:43Z

I submited PR #48 to address the issues with hidden and display attributes. I left the EncodeURI as is.

bicknellr · 2015-11-30T18:37:18Z

Yeah, you're right, expecting the input to be encoded beforehand is more reasonable (and is the behavior of the default img tag anyways).

googlebot · 2015-11-30T18:38:49Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

bicknellr · 2015-11-30T20:13:56Z

@opus1269 I merged your changes and rebased on master - this seems to have confused @googlebot. Could you reply and let it know that you're ok contributing these changes?

opus1269 · 2015-11-30T20:58:17Z

I authored these commits.

On Mon, Nov 30, 2015 at 12:38 PM googlebot [email protected] wrote:

We found a Contributor License Agreement for you (the sender of this pull
request) and all commit authors, but as best as we can tell these commits
were authored by someone else. If that's the case, please add them to this
pull request and have them confirm that they're okay with these commits
being contributed to Google. If we're mistaken and you did author these
commits, just reply here to confirm.

—
Reply to this email directly or view it on GitHub
#44 (comment)
.

bicknellr · 2015-11-30T21:53:09Z

Ok, well @googlebot seems to not be able to handle this, I've looked up that you signed it anyways so it should be alright.

cdata · 2015-12-08T22:32:59Z

iron-image.html

-      },
+        img.onload = function() {
+          var resolvedSrc = Polymer.ResolveUrl.resolveUrl(this.src, this.ownerDocument.baseURI);
+          if (img.src !== resolvedSrc) return;


It might be nice to refactor this into a protected method that lets you compare srcs. Something like..

if (this._matchesSrc(img.src)) return;

WDYT?

Sure, I'll add this in.

…stead of testing.

alice · 2015-12-10T23:02:24Z

iron-image.html

-    <div id="placeholder" hidden$="[[_computePlaceholderVisibility(fade,loaded,preload)]]" class$="[[_computePlaceholderClassName(fade,loaded,preload)]]"></div>
-    <content></content>
+    <div id="sizedImgDiv" role="img" aria-label$="{{alt}}" hidden$="[[_computeBackdropHidden(sizing)]]"></div>
+    <img id="img" alt$="{{alt}}" hidden$="[[_computeImgHidden(sizing)]]">


This looks really nice.

However, one thing I hadn't considered is that this means the default case will be that this will be

<img alt="" src=...>

This means that unless the user specifies an alt attribute, the image will be hidden from screen readers (won't show up at all, as if it's not there to begin with).
Is there some way to set the default value to be null and judiciously add this attribute only if the alt attribute on the host is non-null?

Similarly, if the user does set alt="" on the host, #sizedImagDiv should probably be aria-hidden, to get the same effect.

(For extra credit, it would be great to set a placeholder aria-label on #sizedImgDiv in that case. For <img> elements, screen readers do the job on their own if no alt attribute is provided, but in this case it won't have enough information to do that since this isn't an <img>. The usual fallback is the filename part of the src.)

So aria-label="" doesn't hide an element from screen readers like alt="" would?

Nope - with ARIA everything needs to be explicit. With native attributes there's a bunch of implicit stuff that goes on that you just kinda ... need to memorise. :\

Ok, updated again. I wrote some tests too but they're really implementation dependent. Is there any way to programmatically inspect the accessibility tree?

You can't directly programmatically inspect the accessibility tree just yet - there are a couple of experimental APIs around but not really in any usable state for your purposes.

The Accessibility Developer Tools does have some utils for trying to work out the computed name and role of a particular element:

https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/js/AccessibilityUtils.js#L626
https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/js/Properties.js#L236

Obviously in this case you'd still need to look at the child nodes inside shadow DOM, though.

Also, this looks really great now! I love all the tests.

Nifty; @cdata can you take a look again?

cdata · 2015-12-14T20:32:34Z

iron-image.html

+          reflectToAttribute: true
+        },
+
+        _imgDivARIALabel: {


Should this be private?

Yeah, this isn't meant to be set from the outside. It's just part of a bug work-around.

(gone now anyways)

bicknellr · 2015-12-14T22:04:31Z

@cdata one more time?

kevin-wise · 2016-01-07T21:22:03Z

@cdata Is there any chance this could get merged soon? We are waiting on this for #33.

abdonrd · 2016-01-07T21:47:05Z

+1

BartKoppelmans · 2016-01-13T18:03:08Z

+1

opus1269 · 2016-01-13T18:07:40Z

👍

cdata · 2016-01-13T18:26:06Z

iron-image.html

-      _computeRequiresPreload: function() {
-        return this.preload && !this.loaded;
+        var pathComponents = (new URL(this._resolveSrc(this.src))).pathname.split("/");
+        return pathComponents[pathComponents.length - 1];


cdata · 2016-01-13T18:51:19Z

Small details, otherwise LGTM 👍

Fixes issues with placeholder state and url; adds error property.

googlebot added the cla: yes label Nov 20, 2015

bicknellr force-pushed the bicknellr/state branch from e053b8a to 2eccb5a Compare November 20, 2015 23:08

bicknellr assigned cdata Nov 20, 2015

bicknellr force-pushed the bicknellr/state branch 4 times, most recently from f066852 to 78358f7 Compare November 23, 2015 19:42

googlebot added cla: no and removed cla: yes labels Nov 30, 2015

bicknellr and others added 3 commits November 30, 2015 11:31

Fixes issues with placeholder state and url; adds error property.

ebac79f

fix setting hidden and display attributes, JSHint fix

ef71fef

remove encodeURI - causes double encode on 500px

3e4fb02

bicknellr force-pushed the bicknellr/state branch from b91074f to 3e4fb02 Compare November 30, 2015 20:03

bicknellr mentioned this pull request Nov 30, 2015

Preload placeholder image not being properly displayed - Fix issue #23 #25

Closed

davidrissato mentioned this pull request Dec 3, 2015

Preload placeholder image not being properly displayed #23

Closed

cdata reviewed Dec 8, 2015
View reviewed changes

backdrop -> sizedImgDiv

4c5e605

bicknellr mentioned this pull request Dec 8, 2015

Added the ability to fall back to another image (fixes #33) #34

Closed

bicknellr added 3 commits December 9, 2015 16:04

_matchesImgSrc -> _resolveSrc; _resolveSrc now only resolves in…

b27af65

…stead of testing.

Adds aria-hidden="true" to internal img.

0f9633e

Accessibility updates.

c87d9fe

bicknellr mentioned this pull request Dec 10, 2015

add error property for invalid src #35

Closed

alice reviewed Dec 10, 2015
View reviewed changes

Accessibility updates.

0b1acb9

bicknellr force-pushed the bicknellr/state branch from 4d0b237 to 0b1acb9 Compare December 11, 2015 03:18

cdata reviewed Dec 14, 2015
View reviewed changes

alt now defaults to null.

11f680e

cdata mentioned this pull request Dec 16, 2015

Add alt attribute for basic accessibility #50

Closed

cdata reviewed Jan 13, 2016
View reviewed changes

Remove 0.0.0.0 from test URLs.

e49694e

bicknellr added a commit that referenced this pull request Jan 13, 2016

Merge pull request #44 from PolymerElements/bicknellr/state

c95d43d

Fixes issues with placeholder state and url; adds error property.

bicknellr merged commit c95d43d into master Jan 13, 2016

bicknellr deleted the bicknellr/state branch January 13, 2016 22:49

bicknellr mentioned this pull request Jan 13, 2016

Docs: computed props should be private or have doc strings. #53

Closed

bicknellr mentioned this pull request Mar 29, 2016

Put <content></content> at end of image template #72

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issues with placeholder state and url; adds error property. #44

Fixes issues with placeholder state and url; adds error property. #44

bicknellr commented Nov 20, 2015

opus1269 commented Nov 25, 2015

bicknellr commented Nov 25, 2015

opus1269 commented Nov 27, 2015

opus1269 commented Nov 27, 2015

bicknellr commented Nov 30, 2015

googlebot commented Nov 30, 2015

bicknellr commented Nov 30, 2015

opus1269 commented Nov 30, 2015

bicknellr commented Nov 30, 2015

cdata Dec 8, 2015

bicknellr Dec 8, 2015

alice Dec 10, 2015

bicknellr Dec 10, 2015

alice Dec 11, 2015

bicknellr Dec 11, 2015

alice Dec 11, 2015

alice Dec 11, 2015

bicknellr Dec 12, 2015

cdata Dec 14, 2015

bicknellr Dec 14, 2015

bicknellr Dec 14, 2015

bicknellr commented Dec 14, 2015

kevin-wise commented Jan 7, 2016

abdonrd commented Jan 7, 2016

BartKoppelmans commented Jan 13, 2016

opus1269 commented Jan 13, 2016

cdata Jan 13, 2016

cdata commented Jan 13, 2016

Fixes issues with placeholder state and url; adds error property. #44

Fixes issues with placeholder state and url; adds error property. #44

Conversation

bicknellr commented Nov 20, 2015

opus1269 commented Nov 25, 2015

bicknellr commented Nov 25, 2015

opus1269 commented Nov 27, 2015

opus1269 commented Nov 27, 2015

bicknellr commented Nov 30, 2015

googlebot commented Nov 30, 2015

bicknellr commented Nov 30, 2015

opus1269 commented Nov 30, 2015

bicknellr commented Nov 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicknellr commented Dec 14, 2015

kevin-wise commented Jan 7, 2016

abdonrd commented Jan 7, 2016

BartKoppelmans commented Jan 13, 2016

opus1269 commented Jan 13, 2016

Choose a reason for hiding this comment

cdata commented Jan 13, 2016