335689 - [FIXr]Styles registered with stylesheet service should apply to already loaded documents

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jason Barnabe (np)]

nsIStylesheetService allows you to register user stylesheets. These stylesheets only take effect on documents loaded after the registration. There's no way to have it apply to documents loaded before registration short of reloading them.

To make the interface more useful, when registering a stylesheet, it should also be applied to documents that are already loaded.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Should unregistering a sheet deapply it from already-loaded documents too?

Comment 2

[Jason Barnabe (np)]

Yeah, I was just about to mention that.

Comment 3

[(not currently active) Ted Mielczarek]

We defined this as out of scope when we implemented it originally.  :)  I agree that it would be useful, it's probably just beyond my ability to implement it in that way.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Jason, want to test this?

Comment 5

[Jason Barnabe (np)]

Attachment: testcase (must be loaded as chrome)

I opened the testcase as chrome in one tab and Google in the other. I checked off the box in the testcase which registered a style that should make everything red. Nothing changed. I opened a new tab, again going to Google, and everything was red there. I unchecked the checkbox in the testcase, which unregisters the style, and the red Google tab didn't switch back to normal colors.

Unless I'm missing something, there doesn't seem to be a difference in behaviour with this patch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: With feeling

Observer notifications don't work if you don't call AddObserver!  News at 11.

Does this work?  ;)

Comment 7

[Jason Barnabe (np)]

Still nothing.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: And this?

I finally got the testcase to work, and this patch definitely works with that testcase over here.  Then again, so does the previous one, but this one has a change that should make some compilers happier, maybe...

Comment 9

[Jason Barnabe (np)]

Attachment: test version of stylish

Sorry, I was just not building correctly. It seems to work perfectly fine. Attached is a version of the Firefox extension Stylish. This version solely uses the stylesheet service to apply styles, and with this patch, they are applied and unapplied without requiring a reload.

Performance-wise it takes ~3 seconds to do either action with a handful of tabs open, which is comparable to the time it took previous versions of Stylish to apply or remove a html:link to every open document.

Nit: you've misspelled "successful" in your patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 220100 [details] [diff] [review]
And this?

Yeah, unfortunately if we do this it does require us to reresolve all style in all open things...  Not much I can do about making that faster offhand...

I'll fix the spelling.

Comment 11

[Jesse Ruderman]

Can it be done later in background tabs?  (I think there's an optimization like that when the window is resized.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Maybe...  It could easily screw up scripts to do that, though, so doing it right is a lot of work, and since I don't use tabs much myself I have pretty low motivation for that. ;)  At least in this bug.

Comment 13

[Jesse Ruderman]

> Maybe...  It could easily screw up scripts to do that, though, so doing it
> right is a lot of work

Can you elaborate?

> since I don't use tabs much myself I have pretty low motivation for that

Is there an API in Gecko to determine whether a window is completely obscured?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Can you elaborate?

If script running in a tab asks for the styles for something, it should probably get the current styles for that thing, background tab or no.  Unless you can force that script to be rerun when the tab is shown...

> Is there an API in Gecko to determine whether a window is completely obscured?

See what the code in nsViewManager::SetWindowDimensions does.

Comment 15

[Jesse Ruderman]

> If script running in a tab asks for the styles for something, it should
> probably get the current styles for that thing, background tab or no.  Unless
> you can force that script to be rerun when the tab is shown...

How do we solve that problem for the window-resize optimization?

Comment 16

[Boris Zbarsky [:bzbarsky]]

We perform the resize.  Scripts that care about window size always watch resize events.  There are no equivalent events for style changes, really.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 220100 [details] [diff] [review]
And this?

You should call mStyleSet's BeginUpdate and EndUpdate methods at the start and end of AddUserSheet -- otherwise it could do a lot of rule processor gathering, no?  (If not, assert that it's batching.)

Have you tested that the ordering stuff that you commented about really works?  I didn't bother following through the twisty maze of Append vs. Prepend calls, or checking that forward iteration was the right way -- but please check that you chose those the right way in AddAgentSheet and AddUserSheet.  It might also be good for PresShell::AddUserSheet to assert that aSheet doesn't get removed but it does get added.  And for both nsStyleSet::mSheets (and nsCSSRuleProcessor::mSheets, probably by reference) to document what the order of the sheets means.

Any idea what the assertion in nsStyleSet::RemoveStyleSheet about completeness is about and whether you could trigger it?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> You should call mStyleSet's BeginUpdate and EndUpdate methods 

Good catch.

> but please check that you chose those the right way in AddAgentSheet and
> AddUserSheet.

Urgh.  Good catch.  I was iterating in the wrong direction when prepending... Will fix.

> It might also be good for PresShell::AddUserSheet to assert that aSheet
> doesn't get removed but it does get added.

I can't assert the latter in the face of OOM.  I could assert the former just by checking whether aSheet is in the set before I start removing, I suppose.  It doesn't seem that useful, but let me know if you want me to do this and I will.

> Any idea what the assertion in nsStyleSet::RemoveStyleSheet about completeness
> is about

Yeah.  When I added the idea of incomplete sheets I put asserts in all the style set methods that we're not working with such sheets in the set.  There's no way to trigger it without triggering the asserts on adding incomplete sheets to the set, to my knowledge.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 21

[Jason Barnabe (np)]

Comment on attachment 224821 [details] [diff] [review]
Updated to comments

I'd like this on the 1.8.1 branch because it greatly improves the usefulness of this interface.

Comment 22

[Boris Zbarsky [:bzbarsky]]

You want to request the approval from someone...  And note that this is technically an API change,  It does make the API more useful, but still...

I probably should have revved the IID, actually.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do you think it could break any existing callers?  Or is it just that you think new callers may want to test for the new behavior being present?  Technically we're supposed to avoid such API changes...

Comment 24

[Jason Barnabe (np)]

Stylish registers sheets with the stylesheet service, but also puts a html:link in every document to make the style apply immediately. It's not a perfect solution because each method applies the sheets with a different level of importance, but it's better than nothing. If the new functionality is present, nothing would break. If I could test for it, though, I could skip the html:link part and get consistent results.

Comment 25

[Boris Zbarsky [:bzbarsky]]

David, I doubt existing callers would really depend on the current behavior, though I wouldn't put anything past them, frankly.  The main reasons to change the IID would be so that callers could test for the old behavior and supply workarounds if desired.

I'm not sure I would be happy taking this change on the 1.8 branch, frankly.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: UUID change if we decide to allow testing for the changed behavior

Comment 27

[(not currently active) Ted Mielczarek]

As much as I'd like to see this on the 1.8 branch, it definitely changes the contract for this interface.  I don't think we can take it and claim unchanged APIs.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 224821 [details] [diff] [review]
Updated to comments

Bugzilla users in
general shouldn't make approval requests; they should either (1) make blocking
requests or (2) ask the patch author to make an approval request if the patch
author thinks the patch is appropriate for whatever branch.  One of the
principles of requests like this is that two people have to agree:  the patch
author and one of the author's peers.  Other users requesting approval skips
that step: you get only one person, the peer, and not the author.

If bzbarsky re-requests, I'll reconsider, but he seems slightly against.  I don't care so much about the interface issue...

Comment 29

[Jason Barnabe (np)]

What about the IID change?

Comment 30

[Christian Persch (GNOME) (away; not receiving bug mail)]

Since now stylesheet addition and removal are applied immediately, *reloading* a sheet (which I do by removing it and adding it again) now causes 2 of those (slow) updates. Would it be possible to add a ReloadSheet function to nsIStyleSheetService which combines that into only one update to all open pages? Should I file a new bug for that?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 225198 [details] [diff] [review]
UUID change if we decide to allow testing for the changed behavior

I think we should take the IID change on trunk.

Comment 32

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #30)
> Since now stylesheet addition and removal are applied immediately

chpe, they're applied off an event; the event is now posted immediately.  There is only one change, if nothing weird is happening.  Are you actually seeing the remove/add pattern take twice as much time now as it used to?  If so, I'd love a testcase (extension, xulrunner app, whatever) so I can debug what's going on.

That's not to say that we shouldn't have a ReloadSheet function, just from the point of view of API nicety.  But performance shouldn't be a driving reason for it.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Checked in the iid change on trunk.

Comment 34

[Eric Shepherd [:sheppy]]

The article covering nsIStyleSheetService has a note now indicating that Firefox 3 applies the style immediately; I'm marking this as doc complete.