Problem/Motivation
The bug was discovered while backporting to Navbar. A tray without a heading will raised a JS error because we attempt to access to the text node of the element before establishing if the element exists.
The image above shows the Inspector in Chrome just as the error is about to occur in the onActiveTray
method in navbar.js.
Proposed resolution
Add a check for the element before accessing its text content.
User interface changes
None.
API changes
None.
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedComment #2
Sam152 CreditAttribution: Sam152 commentedI had a look at this issue. The biggest issue was figuring out what to say when a tray had no heading. I considered falling back on the anchor text, however that doesn't seem very consistent, so I settled on "Tray opened." and "Tray closed.".
I have attached a patch to get the ball rolling, but if anyone has any feedback on what the fallback dictation should be, I can look at the patch again.
Comment #3
Sam152 CreditAttribution: Sam152 commentedComment #5
Sam152 CreditAttribution: Sam152 commented2: 2141417-trays-without-headings-javascript-error-2.patch queued for re-testing.
Comment #6
nod_Comment #7
babruix CreditAttribution: babruix commentedNot sure how to test it..
Comment #8
Sam152 CreditAttribution: Sam152 commentedYou can test it by adding a log to "Drupal.announce" and then opening and closing the toolbar. Everything that the aural view announces will be outputted to the console.
Comment #10
Sam152 CreditAttribution: Sam152 commented7: 2141417-trays-without-headings-javascript-error-7.patch queued for re-testing.
Comment #11
Sam152 CreditAttribution: Sam152 commentedComment #12
Wim LeersStrange use of parentheses here.
Never use string concatenation for translation. That breaks translations. See https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi....
Comment #13
jessebeach CreditAttribution: jessebeach commentedSo in the string case, we'd want to see something like this:
text = Drupal.t('Tray @action', {'@action': action});
Comment #14
Sam152 CreditAttribution: Sam152 commentedI have attached two patches which look at the given feedback. In patch b, I have translated the 'closed' or 'opened' string before passing it in as an argument to the later translate function. Not sure if this is the right approach, however I feel that those two string would need translating at some point?
Comment #15
droplet CreditAttribution: droplet commentedDon't use expression inside Drupal.t.
Please move "var text" to top :)
Comment #16
babruix CreditAttribution: babruix commentedMoved "var text" to top.
Comment #17
babruix CreditAttribution: babruix commentedComment #18
nod_Else on a new line please :)
Comment #19
droplet CreditAttribution: droplet commentednull ? Drupal.t('closed') : Drupal.t('opened')
:)
Comment #20
droplet CreditAttribution: droplet commentedComment #21
babruix CreditAttribution: babruix commentedChange from comment #19.
Comment #22
nod_Still need the else on a new line, and while we're at it keep the one var per variable please.
Have a look at the rest of the core JS files please.
Comment #23
babruix CreditAttribution: babruix commentedComment #24
Wim LeersSo sorry that this had to sit here for well over six months without review :(
Still applies cleanly, and still works great. Thanks!
Comment #25
alexpottThis issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed e25acf2 and pushed to 8.0.x. Thanks!