fix: read the associated label's text in inputLabel() - #815
Merged
Conversation
jQuery's .val() getter reads elem.value, and a <label> has no value property, so the lookup always returned "" and every input imported from an HTML5 <menu> silently fell back to its name attribute. Read .text() instead, trim it, and only fall back to the name attribute when there is no label text at all. The wrapping <label> path is trimmed the same way, so a whitespace-only wrapper no longer wins over the name attribute. The id is now escaped before it goes into the attribute selector, and the html5 polyfill's [contextmenu=<id>] selector is quoted and escaped too: an id holding a quote or a CSS metacharacter used to produce a malformed selector and throw. $.escapeSelector() only exists in jQuery 3+, so the escaping is done locally. Fixes #811
That selector string doubles as the registration key (namespaces[o.selector]),
so quoting it would silently stop matching for anyone passing the old literal
into $.contextMenu('destroy'/'update', ...). Backwards compatibility wins over
the hygiene fix; the escaping stays where it is safe, on the label[for="..."]
lookup, which is built and consumed internally and never stored.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #811
Heads up: this is an intentional, user-visible behaviour change
Worth a release note.
inputLabel()has effectively returnednode.namefor every input for years, because the label lookup never worked. With the lookup fixed, anyone importing an HTML5<menu>with labelled inputs will see different menu item text after a routine update: the label text instead of thenameattribute. That is the bug being fixed, but users who compensated by naming their inputs to read nicely will notice it.Nothing is lost, only upgraded. The
node.namefallback is preserved exactly as-is for every case where it applies today:<label>with textnameattribute<label>that is empty or whitespace-onlynameattributenameattributenameattributenameattributenameattributenameattributeNo public signature or return type changed.
inputLabel()is private, and the two helpers added are private too.The bug
inputLabel()read the associated<label>with.val():jQuery's
.val()getter readselem.value. A<label>has novalueproperty, so the getter always returned"", which is falsy, so the expression always fell through tonode.name. The label text was never used, for any input, ever.inputLabel()is the fallback name for every<input>,<select>and<textarea>imported from an HTML5<menu>($.contextMenu.fromMenu()and$.contextMenu('html5')), so those menus showed rawnameattributes where they should have shown human readable labels.The fix
.text()instead of.val(), on the first matching label (an input may legally have more than one label, and concatenating them would produce garbage likeFirstSecond).Trimming. Label markup almost always carries surrounding whitespace, so the text is trimmed. A label that is empty or whitespace-only is deliberately treated as no label at all and the
nameattribute is used, so trimming can never turn a working menu item into a blank one. The wrapping<label> text <input>path inmenuChildren()is trimmed the same way for consistency; previously a whitespace-only wrapper was truthy and beat thenamefallback, which is the one case where trimming makes an item gain a name it did not have.$.trim()is deprecated in jQuery 3 and gone in jQuery 4, andString.prototype.trimis missing in the oldest browsers jQuery 1.12 still runs on, so trimming is done with a small local helper.Secondary item from the issue: only half of it is in
The issue's "Secondary, minor" section flagged two places that concatenate a DOM id into a selector. They are treated differently here, on purpose.
Escaped (in this PR): the
label[for="…"]lookup ininputLabel(). That selector string is built and consumed inside a single function call, never registered, stored or handed back to callers, so escaping it is pure upside: an id containing a quote or a CSS metacharacter used to make jQuery throw, and now it does not.Deliberately not escaped:
[contextmenu=' + this.id + ']'in thehtml5polyfill. That string doubles as the registration key. Menus are keyed by their literal selector (namespaces[o.selector]), so changing what the polyfill registers from[contextmenu=myid]to[contextmenu="myid"]would silently stop matching for anyone who later passes the old literal into$.contextMenu('destroy', '[contextmenu=myid]')or'update'. Silent breakage on a routine update is exactly what we want to avoid, and it is unrelated hygiene bundled into a functional bugfix, so it is left exactly as it is onmasterwith a comment explaining why. An id holding a CSS metacharacter therefore still throws there; that is a separate concern, worth its own issue if anyone hits it.$.escapeSelector()is not used for the escaping that did land, guarded or otherwise, since it only exists in jQuery 3+ and this plugin supports jQuery 1.12+. A quoted CSS attribute value only needs quotes, backslashes and newlines escaped, which is allescapeAttributeValue()does: a self-contained ~8 line function with no version-dependent branches, so it behaves identically on every supported jQuery. The full unit suite was run locally against jQuery 1, 2, 3 and 4, and CI covers the same matrix.Tests
test/unit/issue-811-input-label.test.js, 16 tests across the real import paths ($.contextMenu.fromMenu()and$.contextMenu('html5', true)), not the private helper:<label for="...">nameattribute (the backwards-compatibility cases above)<menu>is still found<label>still wins and is trimmed; a whitespace-only wrapper falls back tonamehtml5polyfill still registers and opens a menu, and an input imported through the polyfill shows its label rather than itsnameEvery one of these fails on
master. Full suite green: 96 unit tests on jQuery 1, 2, 3 and 4, plus the 36 acceptance tests, which are unaffected.Docs:
documentation/docs/html5-polyfill.mdnow states where an imported input's name comes from, and the changelog entry calls out the visible change.