Closed Bug 900046 Opened 11 years ago Closed 11 years ago

Changing between tabs causes entire GUI to reload with default motif before applying user motif

Categories

(Other Applications :: ChatZilla, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: thorbear, Assigned: xabolcs)

References

Details

(Whiteboard: [cz-0.9.91])

Attachments

(3 files, 1 obsolete file)

Attached file metro.css
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Using a motif that changes other GUI-elements than just the chat, changing between tabs (Ctrl+Tab).
FireFox 22.0
ChatZilla 0.9.90.1
Windows 7 x64


Actual results:

The GUI flashes into the default motif before returning to the user motif, which also makes the whole window "shake".


Expected results:

The transition between tabs should just change the contents of the userlist and chat.
Yeah, we now always reappend this node, but we actually call updateAppMotif regularly, and should probably just check whether the existing motif is the same as the new one, in which case we shouldn't do anything (rather than reappending the node, which is probably causing blinking/weirdness if fetching the motif is expensive for some reason.
Status: UNCONFIRMED → NEW
Depends on: 888672
Ever confirmed: true
It seems to me this is an easy fix.
In that case can I take it?
(In reply to Szabolcs Hubai (:xabolcs) from comment #2)
> It seems to me this is an easy fix.
> In that case can I take it?

Shouldn't be hard, no. Do check that doing /sync-whatever as a command still overrides this, in case you've changed the motif (but the URL has remained the same). :-)
(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #2)
> > It seems to me this is an easy fix.
> > In that case can I take it?
> 
> Shouldn't be hard, no. Do check that doing /sync-whatever as a command still
> overrides this, in case you've changed the motif (but the URL has remained
> the same). :-)

Well, assuming that worked before the thing that caused bug 888672; I actually guess layout probably optimizes for setting things to an identical value and, in that case, does nothing anyway. In which case we should possibly follow suit.
I can repro this issue with this motif on official release of firefox, and on trunk also with fixed Bug 888864, but i can repro withOUT Bug 888672 patch also. Seems to me adding or removing reappend hack doesn't influence this.
(In reply to :Gijs Kruitbosch from comment #4)
> Well, assuming that worked before the thing that caused bug 888672; I
> actually guess layout probably optimizes for setting things to an identical
> value and, in that case, does nothing anyway. In which case we should
> possibly follow suit.

I hope, I understand you correctly, because I was able to fix this bug with a one-liner. :)

Patch is coming soon.
Assignee: rginda → xabolcs
Status: NEW → ASSIGNED
Reappend node lazily.
Attachment #786032 - Flags: review?(gijskruitbosch+bugs)
This is the patched (w/ patch v1) and repacked version of AMO version of ChatZilla.
Attachment #786036 - Flags: feedback?(utasir)
Attachment #786036 - Flags: feedback?(thorbear)
Comment on attachment 786036 [details]
chatzilla-0.9.90.1 with patch v1 applied

That does indeed appear to fix the issue.
Comment on attachment 786032 [details] [diff] [review]
patch v1 - do not reappend node blindly

Review of attachment 786032 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, thanks!

::: xul/content/static.js
@@ +2414,5 @@
>          {
>              node = document.createProcessingInstruction("xml-stylesheet", dataStr);
>              document.insertBefore(node, document.firstChild);
>          }
> +        else if (node.data !== dataStr)

Nit: no need for strict not-equals.
Attachment #786032 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #10)
> Nit: no need for strict not-equals.

In this case, here is the updated patch.


PS: I'm not good at commit messages, feel free to reword it.
Attachment #786032 - Attachment is obsolete: true
Attachment #786161 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 786161 [details] [diff] [review]
patch v1.1 - do not reappend node blindly

That's fine, I can fix the description on checkin, no worries. :-)
Attachment #786161 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 786036 [details]
chatzilla-0.9.90.1 with patch v1 applied

Szabi, It's good, I removed ?feedback
Only 1 notice: the subject of patch "do not reappend node blindly"
can be "do not redefine stylesheet blindly" As we talked via email, it's independent from reappend issue based on my Comment 5.
Attachment #786036 - Flags: feedback?(utasir)
Attachment #786036 - Flags: feedback?(thorbear)
Moar patches!
Flags: needinfo?(gijskruitbosch+bugs)
http://hg.mozilla.org/chatzilla/rev/c8a6adb73761
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: --- → FIXED
Whiteboard: [cz-0.9.91]
Whiteboard: [cz-0.9.91]
Whiteboard: [cz-0.9.91]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: