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

RESOLVED FIXED

Status

Other Applications
ChatZilla
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Thorbear, Assigned: xabolcs)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.91])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 783799 [details]
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.

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
It seems to me this is an easy fix.
In that case can I take it?

Comment 3

5 years ago
(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). :-)

Comment 4

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Comment 7

5 years ago
Created attachment 786032 [details] [diff] [review]
patch v1 - do not reappend node blindly

Reappend node lazily.
Attachment #786032 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 8

5 years ago
Created attachment 786036 [details]
chatzilla-0.9.90.1 with patch v1 applied

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)
(Reporter)

Comment 9

5 years ago
Comment on attachment 786036 [details]
chatzilla-0.9.90.1 with patch v1 applied

That does indeed appear to fix the issue.

Comment 10

5 years ago
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+
(Assignee)

Comment 11

5 years ago
Created attachment 786161 [details] [diff] [review]
patch v1.1 - do not reappend node blindly

(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 12

5 years ago
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)

Comment 14

4 years ago
Moar patches!
Flags: needinfo?(gijskruitbosch+bugs)

Comment 15

4 years ago
http://hg.mozilla.org/chatzilla/rev/c8a6adb73761
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: --- → FIXED

Updated

4 years ago
Whiteboard: [cz-0.9.91]

Updated

3 years ago
Whiteboard: [cz-0.9.91]

Updated

3 years ago
Whiteboard: [cz-0.9.91]
You need to log in before you can comment on or make changes to this bug.