260657 - Txul, Ts regression caused by checkbox/radio underlines

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Robert Kaiser]

The recent checkin for bug 68841 caused a significant increase in new XUL window
(Txul) and startup (Ts) times in multiple tinderboxen. I verified those
regressions on comet, luna (both Linux), monkey and silverstone (both MacOSX) -
just look at their graphs. the only checkin that happened in the the time all
four of then regressed was bug 68841 - so it seems a clear thing where it came from.
(Note that the Linux boxes showed the increase for Ts as well, while Mac boxes
didn't really, all boxes show the Txul increase though.)

Some numbers:
comet - before: Txul:348ms / Ts:1167ms
comet - after:  Txul:411ms / Ts:1241ms
luna - before: Txul:778ms / Ts:3160ms
luna - after:  Txul:894ms / Ts:3299ms
monkey - before: Txul:485ms / Ts:5928ms
monkey - after:  Txul:518ms / Ts:5999ms
silverstone - before: Txul:474ms / Ts:3303ms
silverstone - after:  Txul:504ms / Ts:3267ms

Comment 1

[Aaron Leventhal]

I have some ideas.

Comment 2

[Aaron Leventhal]

Attachment: Remove impact on main window and most widgets. Accesskey binding only happens for labels with a control

Comment 3

[Aaron Leventhal]

Comment on attachment 159588 [details] [diff] [review]
Remove impact on main window and most widgets. Accesskey binding only happens for labels with a control

Duh, we should have put the code in the label-control binding in the first
place.

Comment 4

[Aaron Leventhal]

Comment on attachment 159588 [details] [diff] [review]
Remove impact on main window and most widgets. Accesskey binding only happens for labels with a control

Duh, we should have put the code in the label-control binding in the first
place.

Comment 5

[[e-mail]]

Comment on attachment 159588 [details] [diff] [review]
Remove impact on main window and most widgets. Accesskey binding only happens for labels with a control

>+      <property name="control" onget="return null;">
That control code looks wacky, do you have a test case for that?

>-            if (!accessKey || !labelText) {
>+            if (!accessKey || !labelText || !control) {
Did this get left over from a previous fix perhaps?

Comment 6

[Aaron Leventhal]

(In reply to comment #5)
> (From update of attachment 159588 [details] [diff] [review])
> >+      <property name="control" onget="return null;">
> That control code looks wacky, do you have a test case for that?
This is on purpose. In this binding I know there is no control attribute, so
this is just more efficient than saying |return getAttribute('control')|
If there was a control attribute we'd pick up the real control getter under
#label-control

> 
> >-            if (!accessKey || !labelText) {
> >+            if (!accessKey || !labelText || !control) {
> Did this get left over from a previous fix perhaps?
This is new. The idea with this new patch in general, is that if there is no
control there should be no accesskey underlining. 

Comment 7

[[e-mail]]

(In reply to comment #6)
>This is on purpose.
I'm sure it is, but I want to see it work ;-)

>>>-            if (!accessKey || !labelText) {
>>>+            if (!accessKey || !labelText || !control) {
>>Did this get left over from a previous fix perhaps?
>This is new. The idea with this new patch in general, is that if there is no
>control there should be no accesskey underlining. 
a) if there is no control, then this binding won't get used b) radio labels?

Comment 8

[Aaron Leventhal]

(In reply to comment #7)
> (In reply to comment #6)
> >This is on purpose.
> I'm sure it is, but I want to see it work ;-)
Test case coming.

> 
> >>>-            if (!accessKey || !labelText) {
> >>>+            if (!accessKey || !labelText || !control) {
> >>Did this get left over from a previous fix perhaps?
> >This is new. The idea with this new patch in general, is that if there is no
> >control there should be no accesskey underlining. 
> a) if there is no control, then this binding won't get used b) radio labels?
a) Yes it will, when the control is dynamically removed by setting label.control
= '' or where control="foo" and foo isn't valid id in the document. Test case
coming.
b) Radio labels work because of the xul.css change and the bindingParent code

Comment 9

[Aaron Leventhal]

Attachment: Test cases

First 2 test cases show dynamic change of control property works
Last test case shows that label.control == label.getAttribute('control') when
there is no control attribute set. However, I had to change 
+    <property name="control" onget="return null;">
to
+    <property name="control" onget="return '';">

Comment 10

[Aaron Leventhal]

One thing I should mention is that setting
label.control = '' or label.control = '' 
after there was a control before, doesnt change the binding back to text-label

I would probably have to have special code to removeAttribute('control') in
those cases, because setAttribute('') and setAttribute(null) don't seem to do
that. So the binding is still the text-label one at that point.

Also, this.formatAccessKey() is called after the control property setter, so
even if we do that we still have to handle the !control case in formatAccessKey().

Comment 11

[Aaron Leventhal]

Attachment: 1) Also patch xpfe, 2) When no control attribute, control property returns '' instead of null (same as getAttribute('control') would do)

Comment 12

[[e-mail]]

Comment on attachment 159626 [details] [diff] [review]
1) Also patch xpfe, 2) When no control attribute, control property returns '' instead of null (same as getAttribute('control') would do)

I couldn't get a straight answer from jst as to whether getAttribute would
reliably return '' when the attribute was not set. Also, radio buttons stopped
working because they're not labelled control elements (should be a 1-liner).

Comment 13

[Aaron Leventhal]

Attachment: Final patch with changes requested by Neil

Comment 14

[Benjamin Smedberg]

Comment on attachment 159673 [details] [diff] [review]
Final patch with changes requested by Neil

a=me for the toolkit

Comment 15

[Aaron Leventhal]

Checking in toolkit/content/xul.css;
/cvsroot/mozilla/toolkit/content/xul.css,v  <--  xul.css
new revision: 1.41; previous revision: 1.40
done
Checking in toolkit/content/widgets/text.xml;
/cvsroot/mozilla/toolkit/content/widgets/text.xml,v  <--  text.xml
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/content/widgets/radio.xml;
/cvsroot/mozilla/toolkit/content/widgets/radio.xml,v  <--  radio.xml
new revision: 1.10; previous revision: 1.9
done
Checking in xpfe/global/resources/content/xul.css;
/cvsroot/mozilla/xpfe/global/resources/content/xul.css,v  <--  xul.css
new revision: 1.135; previous revision: 1.134
done
Checking in xpfe/global/resources/content/bindings/text.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/text.xml,v  <--  text.xml
new revision: 1.12; previous revision: 1.11
done
Checking in xpfe/global/resources/content/bindings/radio.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/radio.xml,v  <--  radio.xml
new revision: 1.36; previous revision: 1.35
done

Comment 16

[Boris Zbarsky [:bzbarsky]]

Adding the bugs this may have fixed to the dep list.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yeah, all the deps are fixed in today's Linux build.  I'll mark them so.

Comment 18

[Ian Neal]

Relevant parts of patch relanded following landing of aviary branch