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

Implements multi line text input for TextArea #4152

Merged
merged 5 commits into from Dec 5, 2014
Merged

Conversation

mttr
Copy link
Contributor

@mttr mttr commented Nov 29, 2014

Fixes #3918

Can be tested in tests/html/textarea.html. Also implemented some content reflecting IDL attributes for HTMLTextAreaElement while I was in there.

There are some major problems with TextInput when Multiple is enabled that I haven't addressed here, but I'm prepared to open up a follow-up issue.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/3326

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm jdm added S-awaiting-review There is new code that needs to be reviewed. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 30, 2014
@jdm
Copy link
Member

jdm commented Dec 3, 2014

Squash!

@mttr
Copy link
Contributor Author

mttr commented Dec 3, 2014

Was squashing the review commits fine, or should I squash them all?

@jdm
Copy link
Member

jdm commented Dec 4, 2014

That's fine.

@jdm jdm removed the S-needs-squash Some (or all) of the commits in the PR should be combined. label Dec 4, 2014
bors-servo pushed a commit that referenced this pull request Dec 4, 2014
Fixes #3918 

Can be tested in `tests/html/textarea.html`. Also implemented some content reflecting IDL attributes for HTMLTextAreaElement while I was in there.

There are some major problems with TextInput when Multiple is enabled that I haven't addressed here, but I'm prepared to open up a follow-up issue.
@jdm
Copy link
Member

jdm commented Dec 4, 2014

 2:43.00 TEST_START: Thread-TestrunnerManager-3 /html/dom/reflection-forms.html
 2:43.00 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) Full command: /home/servo/buildbot/slave/linux2/build/components/servo/target/servo --cpu --hard-fail http://localhost:8000/html/dom/reflection-forms.html
(pid:8415) "Xlib:  extension "RANDR" missing on display ":0"."
 2:44.23 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "task 'ScriptTask' panicked at 'Expected a UIntAttrValue', dom/element.rs:642"
 2:44.23 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "stack backtrace:"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   1:     0x7f2cb617b600 - rt::backtrace::imp::write::h51da061815fd4fecscq"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   2:     0x7f2cb617e640 - failure::on_fail::h574424058645b8db3xq"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   3:     0x7f2cb619a1a0 - unwind::begin_unwind_inner::h0e863b3f5b6ff01e7Rd"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   4:     0x7f2cb516fa20 - unwind::begin_unwind::h5365247804180897910"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   5:     0x7f2cb54f1820 - dom::element::JSRef<'a, Element>.AttributeHandlers::get_uint_attribute::h0cc6c9e9c24dbdb37P0"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   6:     0x7f2cb5342050 - dom::htmltextareaelement::JSRef<'a, HTMLTextAreaElement>.HTMLTextAreaElementMethods::Cols::h9dc45cec88e5a5bc0y9"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   7:     0x7f2cb5341420 - dom::bindings::codegen::Bindings::HTMLTextAreaElementBinding::get_cols::__rust_abi"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   8:     0x7f2cb53413d0 - dom::bindings::codegen::Bindings::HTMLTextAreaElementBinding::get_cols::h0e3b01b7981cb2cddr2"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "   9:     0x7f2cb6247fd8 - CallJitPropertyOp"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "  10:     0x7f2cb5346230 - dom::bindings::codegen::Bindings::HTMLTextAreaElementBinding::genericGetter::__rust_abi"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "  11:     0x7f2cb53461e0 - dom::bindings::codegen::Bindings::HTMLTextAreaElementBinding::genericGetter::h356f0306f2b28aab3P2"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "  12:     0x7f2cb5ede720 - _ZN2js12InvokeKernelEP9JSContextN2JS8CallArgsENS_14MaybeConstructE"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "  13:     0x7f2cb5edef90 - _ZN2js20InvokeGetterOrSetterEP9JSContextP8JSObjectRKN2JS5ValueEjPS5_S8_"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "  14:     0x7f2cb5e631d0 - _ZN2js5Shape3getEP9JSContextN2JS6HandleIP8JSObjectEES6_S6_NS3_13MutableHandleINS3_5ValueEEE"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "  15:     0x7f2cb5e68d50 - _ZN2js7baseops11GetPropertyEP9JSContextN2JS6HandleIP8JSObjectEES7_NS4_IlEENS3_13MutableHandleINS3_5ValueEEE"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "  16:     0x7f2cb5fa6c20 - _ZN2js4mjit5stubs7GetElemERNS_7VMFrameE"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "  17:                0x0 - <unknown>"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) ""
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "You've met with a terrible fate, haven't you?"
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) ""
 2:44.33 PROCESS_OUTPUT: Thread-TestrunnerManager-3 (pid:8415) "fatal runtime error: Could not unwind stack, error = 5"
 2:44.55 CRASH: Thread-TestrunnerManager-3 pid:14175. Test:None. Minidump anaylsed:False. Signature:[/html/dom/reflection-forms.html]

@jdm jdm added the S-tests-failed The changes caused existing tests to fail. label Dec 4, 2014
@mttr
Copy link
Contributor Author

mttr commented Dec 4, 2014

After modifying the panic! output a bit, I got this:

(pid:7457) "task 'ScriptTask' panicked at 'Expected a UIntAttrValue, got -2147483649; attr name: Atom('cols' type=static)', dom/element.rs:642"

If I'm looking at this particular test correctly, the textview's cols attribute is initialized at 20 here. All I've done with cols is give it the standard make_uint_getter/setter macros. Other than that, I don't touch it at all, so I'm at a complete loss as to what's going on here (not to mention that this somehow passed on mac2).

I mean, I could just get rid of the cols implementation since I'm not even using it right now, but... this is weird.

@mttr
Copy link
Contributor Author

mttr commented Dec 4, 2014

Well, I found the spec for this. If I'm understanding it correctly, element.get_uint_attribute() should be returning either 0 or 1 (depending on whether it's restricted to values greater than 1 or 0......)

...If, on the other hand, it fails or returns an out of range value, or if the attribute is absent, the default value must be returned instead, or 0 if there is no default value.
...If, on the other hand, it fails or returns an out of range value, or if the attribute is absent, the default value must be returned instead, or 1 if there is no default value.

Cols falls into the latter category.

Maybe getting rid of the unneeded uint attributes would be best for now?

@mttr
Copy link
Contributor Author

mttr commented Dec 4, 2014

#4223 should fix this crash if it gets approved.

@mttr
Copy link
Contributor Author

mttr commented Dec 5, 2014

All that, and the answer was right in front of me the the entire time: parse_plain_attribute(). Pushed a fix to the crash, along with a bunch of now-passing WPT's.

@jdm jdm added S-needs-rebase There are merge conflict errors. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 5, 2014
@jdm
Copy link
Member

jdm commented Dec 5, 2014

These changes look good to me, but they need a rebase.

These attributes all reflect their own related content values, with the
exception of defaultValue, which acts as an alias for its IDL
textContent attribute.

Many of these do have default values and constraints which are currently unimplemented.
Addresses reviews

More review addressing
...and passing a whole bunch of new tests.
@mttr
Copy link
Contributor Author

mttr commented Dec 5, 2014

Got it!

@mttr
Copy link
Contributor Author

mttr commented Dec 5, 2014

wait, ugh, something broke again

@mttr
Copy link
Contributor Author

mttr commented Dec 5, 2014

Never mind, forgot to rebuild after the rebase was done. My panic was for nothing. Sorry for the fuss.

bors-servo pushed a commit that referenced this pull request Dec 5, 2014
Fixes #3918 

Can be tested in `tests/html/textarea.html`. Also implemented some content reflecting IDL attributes for HTMLTextAreaElement while I was in there.

There are some major problems with TextInput when Multiple is enabled that I haven't addressed here, but I'm prepared to open up a follow-up issue.
@bors-servo bors-servo closed this Dec 5, 2014
@bors-servo bors-servo merged commit c97a4d9 into servo:master Dec 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement multiline text input
4 participants