Closed Bug 1175399 Opened 9 years ago Closed 9 years ago

Combine readonly attribute consideration into isFocusableElement()

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: timdream, Assigned: timdream)

Details

Attachments

(1 file, 2 obsolete files)

Patch v1.2
4.80 KB, patch
janjongboom
: review+
smaug
: feedback+
Details | Diff | Splinter Review 
Comment left on bug 729623 is no longer valid and the if block should be put into isFocusableElement().
Attached patch Patch v1.0 (obsolete) — — Splinter Review 
I've also added a test to ensure we don't receive any notification for readonly inputs.
Attachment #8623507 - Flags: review?(janjongboom)
Attachment #8623507 - Flags: feedback?(bugs)
Comment on attachment 8623507 [details] [diff] [review]
Patch v1.0

Why you want to use getAttribute() and not .readOnly.
Attachment #8623507 - Flags: feedback?(bugs)
Maybe it's handled differently in Chrome code, but <input readonly> would give a falsey value when reading the attribute through getAttribute. So is this code really working?
Flags: needinfo?(timdream)
Comment on attachment 8623507 [details] [diff] [review]
Patch v1.0

This probably means fix to bug 729623 was invalid? Not sure. Let me change to .readOnly instead.

Also this means the test is false positive here. Gonna change both and submit a new patch.
Attachment #8623507 - Attachment is obsolete: true
Flags: needinfo?(timdream)
Attachment #8623507 - Flags: review?(janjongboom)
Attached patch Patch v1.1 (obsolete) — — Splinter Review 
Patch updated to use element.readOnly and I have verified the test assert the context at the right timing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=224c9d7810f8
Attachment #8628685 - Flags: review?(janjongboom)
Attachment #8628685 - Flags: feedback?(bugs)
Comment on attachment 8628685 [details] [diff] [review]
Patch v1.1

Test didn't pass :-/ it did pass locally...
Attachment #8628685 - Attachment is obsolete: true
Attachment #8628685 - Flags: review?(janjongboom)
Attachment #8628685 - Flags: feedback?(bugs)
Attached patch Patch v1.2 — — Splinter Review 
Made a silly mistake on the previous patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3031903f49b
Attachment #8629272 - Flags: review?(janjongboom)
Attachment #8629272 - Flags: feedback?(bugs)
Comment on attachment 8629272 [details] [diff] [review]
Patch v1.2

r=janjongboom
Attachment #8629272 - Flags: review?(janjongboom) → review+
Attachment #8629272 - Flags: feedback?(bugs) → feedback+
https://hg.mozilla.org/mozilla-central/rev/0371fcd2e3e4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: