Last Comment Bug 11011 - (moz-broken) Need pseudo-classes for when alt text is shown (:-moz-alt-text, :-moz-placeholder, :-moz-broken)
(moz-broken)
: Need pseudo-classes for when alt text is shown (:-moz-alt-text, :-moz-placeho...
Status: RESOLVED FIXED
[Hixie-P5] (but blocking some [Hixie-...
: css-moz, css3, dev-doc-complete, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 normal with 3 votes (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
: Hixie (not reading bugmail)
Mentors:
: 10280 65206 (view as bug list)
Depends on: 75185 83774 191839 296309 309076 309141 309146 309237 310447 310662 315541 329896 380144 435203
Blocks: 8131 alttext 180622 301638 304598 12770 23691 34981 70820 71473 114641 bearded-imagemap 268157 285212 288064 309065 395390
  Show dependency treegraph
 
Reported: 1999-07-31 12:32 PDT by Mats Palmgren (:mats)
Modified: 2014-04-26 03:07 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.14 KB, text/html)
1999-07-31 12:34 PDT, Mats Palmgren (:mats)
no flags Details
First cut at a patch (111.35 KB, patch)
2005-05-06 22:40 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Ready to go, I think (119.44 KB, patch)
2005-08-25 18:42 PDT, Boris Zbarsky [:bz]
roc: review+
cbiesinger: review+
Details | Diff | Review
Same thing as diff -w (115.85 KB, patch)
2005-08-25 18:46 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
biesi's review comments of attachment 193878 (2.44 KB, text/plain)
2005-09-04 15:31 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details
Apdated to comments (120.74 KB, patch)
2005-09-05 20:16 PDT, Boris Zbarsky [:bz]
dbaron: superreview+
Details | Diff | Review
Same as diff -w (117.17 KB, patch)
2005-09-05 20:18 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Updated to dbaron's comments (121.63 KB, patch)
2005-09-18 10:56 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Additional patch to fix Tp regression (7.61 KB, patch)
2005-09-18 12:47 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
Details | Diff | Review
Additional additional patch to fix Tp regression some more (4.06 KB, patch)
2005-09-18 14:05 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
roc: superreview+
Details | Diff | Review

Description Mats Palmgren (:mats) 1999-07-31 12:32:53 PDT
The boxes for ALT text (or file basename) from an <IMG> with broken URI
is not layed out properly, they are overlapping.
build id: 1999072908 on Windows 98
Will create attachment with testcase...

Note: clicking the box will not work either but that has been
reported in bug#5349
Comment 1 Mats Palmgren (:mats) 1999-07-31 12:34:59 PDT
Created attachment 1066 [details]
testcase
Comment 2 troy 1999-07-31 15:05:59 PDT
Kipp can verify for sure, but I think that's the way it is supposed to look.
Line height calculations in CSS don't take into account whether the inline
elements have borders
Comment 3 troy 1999-07-31 15:05:59 PDT
Cc'ing David as well, because he probably knows as well
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 1999-08-01 06:01:59 PDT
What you said about line-heights is correct, but I'm not sure how inline
elements with borders applies to images with alt texts.  I would think either
they shouldn't have borders or they should be inline-block.  (I haven't looked
at the test case, though.)
Comment 5 pmock 1999-08-17 18:43:59 PDT
*** Bug 10280 has been marked as a duplicate of this bug. ***
Comment 6 Hixie (not reading bugmail) 1999-08-28 16:04:59 PDT
Alt text for broken images should just be inserted inline and so if they have a
border then there is going to be a border.

Hmm. We may need a pseudo-class that applies to IMG (and image INPUTs) when they
are broken (or images are disabled), such as "-moz-broken". Then we could have:

 a:link img:-moz-broken,
 a:visited img:-moz-broken,
 input[type=image]:-moz-broken {
   border: none;
 }

...in ua.css, which would remove this problem altogether.
Comment 7 Hixie (not reading bugmail) 1999-08-28 16:58:59 PDT
BTW, given the current ua.css, we render the test case correctly. So I am
changing the summary line to say that what we need is a -moz-x pseudo-class
to match images when they are showing their alt text instead of their image.
Then we can use the CSS I give above in ua.css.
Comment 8 Hixie (not reading bugmail) 1999-08-31 09:29:59 PDT
As agreed with Beth, assigning myself as QA contact for all 'alt text' bugs...
Comment 9 kipp 1999-09-17 10:18:59 PDT
Hey troy - can you either WONTFIX this because you don't care, or do what they
suggest...it sounds reasonable to me...thanks
Comment 10 troy 1999-09-17 10:59:59 PDT
It is reasonable, but we're not doing it for the first version
Comment 11 Hixie (not reading bugmail) 1999-10-04 15:37:59 PDT
Based on Troy's comments, verifying. I'll reopen this after we release 1.0.
Comment 12 Hixie (not reading bugmail) 1999-10-22 18:00:59 PDT
Troy: This is going to be a requirement for fixing bug 8131, since when an
image map is broken, we won't want the replacement alt text to be blue or have
the hand cursor.

Could you give me a rough idea of how long this would take to implement?
Comment 13 troy 1999-10-22 21:03:59 PDT
Then maybe we won't be fixing 8131 then. :-)

It's hard to say. All that should be involved is changing the frame construction
code's CantRenderReplacedElement() code to use a pseudo style.

But I think what we'll probably need to do is probe for the pseudo style and if
it exists then discard the current style context and create a new one using the
pseudo style.

I think with Peter's recent changes we can't have non-leaf style contexts that
don't have an associated frame
Comment 14 Hixie (not reading bugmail) 1999-10-22 23:21:59 PDT
> Then maybe we won't be fixing 8131 then. :-)
I somehow *knew* you would say that... ;-)

> I think with Peter's recent changes we can't have non-leaf style contexts
> that don't have an associated frame.
What style context is the alt text frame _currently_ using? And is it not
a leaf style context anyway?

Note, BTW, that this is a request for a pseudo-_class_, not a pseudo-element.
I'm not sure from your comments how different those two cases would be, but
I would have thought that a pseudo-class would be a less trouble that you seem
to indicate. (There is a related bug which is requesting a pseudo-_element_,
that is the pseudo-element used to refer to the place holder frame. This is
not it, though.)


Anyway. Broken IMG elements that USEMAPs or are in A elements currently get no
underlining / a blue border respectively. This bug would let that be solved very
easily. This is clearly not a dogfood item, though, so I'm not reopening it.
Comment 15 troy 1999-10-23 08:09:59 PDT
The alt text frame is really two frames. Depending on whether the image is
block-level or inline, we first create a block or inline frame and then we
create a child text frame.

The block/inline frame is just using the same style context as the image used (I
think). Then we create a text pseudo element style context for the text frame.

Hence my comment about non-leaf style contexts.

As far as it being a pseudo class style. I didn't notice that the first time
around. That probably makes it harder, although, to be honest I don't know much
about how the pseudo class styles work.
Comment 16 Hixie (not reading bugmail) 2000-01-25 13:01:15 PST
This is likely to be specified by CSS3.
Comment 17 Hixie (not reading bugmail) 2000-05-18 14:35:21 PDT
Moving to future...
Comment 18 Hixie (not reading bugmail) 2000-05-18 14:36:00 PDT
Reassigning to Buster...
Comment 19 Hixie (not reading bugmail) 2001-01-13 03:38:28 PST
*** Bug 65206 has been marked as a duplicate of this bug. ***
Comment 20 Kevin McCluskey (gone) 2001-10-04 16:31:38 PDT
Build reassigning Buster's bugs to Marc.
Comment 21 Hixie (not reading bugmail) 2002-01-25 15:53:04 PST
Actually, we need two pseudo-classes:

   :-moz-alt-text
       the element is being replaced by its alt text.

   :-moz-placeholder
       the element is being rendered as a replaced element but the replaced 
       content isn't yet there. i.e., the element is being rendered as a place-
       holder.

These, as mentioned above, are pseudo-*classes*, not pseudo-elements.
Comment 22 Boris Zbarsky [:bz] 2003-02-16 09:57:37 PST
So.... This should be possible to implement once bug 83774 is fixed, since at
that point the content node (which is what style is being matched on) will know
whether the image load succeeded.

One issue will be switching style appropriately, though....
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-08-02 13:40:56 PDT
Need for new pseudo-classes -> style system.

Comment 24 Boris Zbarsky [:bz] 2004-05-13 13:55:57 PDT
There was a thought at some point (dunno whether it got noted down in a bug) to
make this a content state, basically (:-moz-broken and :-moz-loading pseudos
that images could match).
Comment 25 Boris Zbarsky [:bz] 2005-05-06 22:40:31 PDT
Created attachment 182840 [details] [diff] [review]
First cut at a patch

David, if you don't mind I'll steal this.

This patch implements the following:

1)  Three new pseudo-classes:  :-moz-broken, :-moz-suppressed, and
    :-moz-user-disabled.  These correspond, respectively to images that are
    broken (network error, not image data, blocked by security manager, etc),
    images that are suppressed by the user via "block images from this site",
    and images that are disabled entirely via preferencess.  And image that is
    still loading or already loaded is in none of these pseudo-classes.
2)  Dynamic switching of these pseudo-classes when image loads start and stop.
3)  Application of the :-moz-broken pseudo-class to <object>, <applet>, <embed>

    when they would be replaced by their alternate content.  This isn't
perfect,
    but it can be made so once loading for these elements happens from content.

4)  Possibly better fallback for <applet> (see bug 114641).
5)  A value for the "content" property of "-moz-alt-content".  At the moment,
    this is just alt attribute if it's there, and some other things for HTML
    <input type="image"> if it's not.  The impl of this is a little weird -- I
    just enforce that there's nothing after it in the list.  I need to fix it
to
    either be allowed anywhere, or to be only allowed in the first slot.

I didn't really see a huge need to implement a :-moz-loading pseudo-class, and
I was already starting to use a lot of state bits.  For now, I left us showing
the image frame with the loading placeholder while loading.  I think we'll want
that in general, to avoid a reframe on load complete.

I also had to still leave in some quirks "sized box" code we have.  I think
that once we implement inline-block I may be able to eliminate that; will need
to think about it.

For now, this patch actually sets blocked images to display:none (instead of
the current behavior of having an image frame but not painting it).  If layout
really needs to be preserved when images are blocked, we can use
visibility:hidden instead.  If we also want them to be right-clickable, we have
to go back to manually not painting them in nsImageFrame.

Comments on the general approach?  I'm somewhat trying to make our behavior as
similar to what we have now as I can, with the actual work of writing new style
rules, etc, happening in followup bugs.  But at the same time, I'm trying to
remove as much of the C++ special-casing as I can...
Comment 26 Boris Zbarsky [:bz] 2005-05-06 22:43:49 PDT
Also note that the pseudo-classes are not tied to the actual presentation of the
node, but rather to its state.  I can see people who would prefer to show alt
text for suppressed images, etc, so this made more sense to me than having a
:-moz-alt-text pseudo-class.

I'd really like feedback on the general idea, though, before I finish cleaning
up the patch and start testing.
Comment 27 Hixie (not reading bugmail) 2005-05-06 23:21:28 PDT
Please make sure you are familiar with http://www.hixie.ch/specs/alttext .
Comment 28 Boris Zbarsky [:bz] 2005-05-08 16:37:50 PDT
> Please make sure you are familiar with http://www.hixie.ch/specs/alttext .

I just read it again (I've read it several times before).  It helps a little (in
that it gives me an idea of what styles people may want to apply).

It's not clear to me why the proposed spec treates images with size set in CSS
differently from images with the height/width attributes set.  I see no
substantive difference between the two....

It's also not clear why for images with no width/height attribute we start by
showing the alt text.  I'd rather start by showing a "loading" placeholder and
just reflow when the image size is known, instead of having flashing bits of alt
text around...

In any case, if I understand your spec correctly, it requires the following
distinct states to be identified for an image:

1)  Broken (corrupt data?)
2)  Unavailable (404?)
3)  Unsupported
4)  Never tried loading
5)  Blocked
6)  Size known.

(and "none of the above", which is the "loading" state).  The "Never tried
loading" state should be further subdivided (imo) into:

4a) "src" not set
4b) Images are disabled

Presumably images that are security-blocked, or blocked by same-origin policy
would fall somewhere in your setup?

On a side note, do you see any reason the "borderless box" thing can't just be
implemented by using an inline-block with hidden overflow, border:none, and some
content in it?
Comment 29 Hixie (not reading bugmail) 2005-05-08 23:35:29 PDT
> It's not clear to me why the proposed spec treates images with size set in CSS
> differently from images with the height/width attributes set.  I see no
> substantive difference between the two....

That is unintentional. I have rephrased the spec and revved it (1.3).

Note that unless I, or my specs, say something explicitly, you should assume
that an implied difference between two otherwise equivalent things is a mistake
or an oversight. I'm much more likely to have made a mistake than to have
intentionally made the spec do something stupid. :-)


> It's also not clear why for images [that have no dimensions] we start by
> showing the alt text.

Because you have the following scenarios:

 1. loading...               2. loading...     3. loading...
    reflow.                     reflow.           reflow.
    decoding...                 decoding...       alt text (404)
    reflow.                     image.
    alt text (broken image)

In your case and my case they have the same number of reflows except the 404
case, where mine has one fewer reflow.


> I'd rather start by showing a "loading" placeholder and just reflow when the 
> image size is known, instead of having flashing bits of alt text around...

Changing the size of the placeholder vs having the placeholder just be inline
text seems just as bad to me, except that in the text case you don't get further
disturbed if you never get the picture (you only get the icon changing).

Why would you want a "loading" inline-block placeholder instead of a "loading"
inline text placeholder?


> In any case, if I understand your spec correctly, it requires the following
> distinct states to be identified for an image:

As far as I can tell it needs:

   1. Show nothing.
   2. Show placeholder.
   3. Show image.
   4. Show alt text.

Now, in the "show alt text" and "show placeholder" cases, we can use as many or
as few icons as we want. We could have icons for all of these cases:

   Loading:
      Image load not yet sent
      One for each state of the network connection (DNS, HTTP in, out, x% done)

   Network errors:
      Missing (404)
      Access denied (403)
      Or just one per HTTP response code!

   Decoding errors:
      Unknown MIME type
      Corrupt -- we could have one icon per decoding error!

   Not loading:
      Images disabled by user
      Security blocked
      No src="" attribute

You chose which ones you want!

I would recommend just having:

   Loading and not loaded (images disabled, image loading, no src="", etc)
   Unknown image type (e.g. what we would show for MNGs or plugins)
   Errors (network errors, security errors, decoding errors)


> On a side note, do you see any reason the "borderless box" thing can't just be
> implemented by using an inline-block with hidden overflow, border:none, and 
> some content in it?

Nope, indeed that would be exactly what I would render it as. Assuming it is
otherwise an 'inline'. (If the 'display' is something else, then obviously it
would have to match that display -- 'block', 'list-item', 'table-row', etc.)
Comment 30 Boris Zbarsky [:bz] 2005-05-09 13:00:26 PDT
Unfortunately, in Gecko converting from replacement alt text to an actual image
that you show requires more than just reflow.  It requires a change in the
layout object used, which means you have to flush the content sink, etc (because
changing the layout object tree while the DOM tree is in an inconsistent state
is a good way to crash).  So in fact, the possible scenarios are:

So if we start off with showing the alt text, the "reflow" in the second (most
common, imo) scenario is replaced by a much more expensive operation...

I could try implementing the proposal and seeing how pageload is affected, and I
agree that for images that take a long time to get an initial response on (eg
slow DNS) your proposal leads to better results.  I guess I'll try and see.  ;)

> Why would you want a "loading" inline-block placeholder instead of a "loading"
> inline text placeholder?

I'd actually want a "loading" replaced element placeholder, if the performance
issue arises....

> As far as I can tell it needs:

You're talking about policy (what things look like).  I'm talking about
mechanism (what things _could_ look like, depending on the style rules that are
applied).  I want to make sure that the mechanism I provide is sufficient to
implement the policy you want, of course.

In particular, I need to decide on a list of different things that one could
select on and then I'll need that many bits in the content state (one bit for
each one, to say whether we're in that state)...  Though I may be able to cheat
a little and make some bits mean different things depending on whether another
bit is set.  Need to decide on the list of things we want to match on first.  ;)

> (If the 'display' is something else, then obviously it would have to match that
> display -- 'block', 'list-item', 'table-row', etc.)

This goes back to the not-quite-resolved issue I raised with the WG about
replaced elements as table cells, etc.  For now, I'd probably use a completely
non-replaced box styled as inline-block in ua.css; if the site changes the
display, they get a non-replaced box of whatever display type they use.  And if
the site explicitly sets display:inline, they get that too.
Comment 31 Hixie (not reading bugmail) 2005-05-09 14:15:28 PDT
> Unfortunately, in Gecko converting from replacement alt text to an actual 
> image that you show requires more than just reflow. ... I guess I'll try and 
> see.  ;)

Fair enough, I guess. Don't worry about it too much -- when it turns out to be
too hard, please file a bug on making the placeholder box eventually be
completely inline instead of inline-replaced. But mark it P5 Trivial, no rush.


> You're talking about policy (what things look like).

Yes. That's my job here. :-)


> Need to decide on the list of things we want to match on first.  ;)

Well, as I see it at a minimum you need Not loaded yet and Error.

If you want to plan for the future, you could have:

 0x02  Not loaded, not going to be loaded without user intervention.
 0x03  Not loaded, but will be loading eventually.
 0x04  No image: HTTP 204, 205, 404, 410, no src="" attribute.
 0x05  Image format not supported: HTTP 406, unsupported MIME types.
 0x06  Broken image: Other network errors, decoder errors, security errors.

...and for now just check for state | 0x02 and state | 0x04 respectively.

Or you can break it down even further, although frankly I don't see multiple
icons being of much use to the user.


> > (If the 'display' is something else, then obviously it would have to match 
> > that display -- 'block', 'list-item', 'table-row', etc.)
> 
> This goes back to the not-quite-resolved issue I raised with the WG about
> replaced elements as table cells, etc.

I thought we had resolved that actually, but let me know if I didn't get the WG
resolution to you or if the WG resolution wasn't enough (but not in this bug).


> For now, I'd probably use a completely non-replaced box styled as inline-block
> in ua.css; if the site changes the display, they get a non-replaced box of 
> whatever display type they use.  And if the site explicitly sets 
> display:inline, they get that too.

Please file a bug on fixing that eventually too (slightly higher priority than
the earlier one).
Comment 32 Boris Zbarsky [:bz] 2005-08-25 18:42:34 PDT
Created attachment 193878 [details] [diff] [review]
Ready to go, I think

This is basically done.  I aimed to change current behavior as little as
possible, and have tested using
<http://web.mit.edu/bzbarsky/www/testcases/image-loading/>.  The only three
behavior changes I'm aware of are:

1)  We now show the alt text for <input type="image" src="no such image"
    value="alt text"> in quirks mode instead of just showing a placeholder.
2)  Images loaded via <embed> or <object> end up firing onload twice because we

    reframe _after_ the frame is constructed.  This will become a nonissue once

    object loading moves to content.
3)  Suppressed images are now completely suppressed (display:none) instead of
    just being a blank space.  We'll have to see what users think of this
    setup...

The patch has three parts, basically: content changes, layout changes, style
system changes.  I'd like David to look over this last set and as much of the
rest as he wants, and I'd like roc to review the layout changes and biesi to
review the content changes.
Comment 33 Boris Zbarsky [:bz] 2005-08-25 18:46:38 PDT
Created attachment 193880 [details] [diff] [review]
Same thing as diff -w

This makes some of the nsCSSFrameConstructor::ConstructHTMLFrame changes much
simpler to review.
Comment 34 Christian :Biesinger (don't email me, ping me on IRC) 2005-08-28 11:21:06 PDT
Comment on attachment 193878 [details] [diff] [review]
Ready to go, I think

+   * UpdateState recomputes the current state of this image loading content
and
+   * updates what State() returns accordingly.

there is no State() in this class...

+  if (broken && NS_PTR_TO_INT32(broken)) {

the two parts of that if test exactly the same thing, don't they?
Comment 35 Boris Zbarsky [:bz] 2005-08-28 11:58:16 PDT
> there is no State() in this class...

Will fix.

> the two parts of that if test exactly the same thing, don't they?

Indeed.  I'll just test the latter.  Not sure what I was thinking when I wrote
this.  ;)
Comment 36 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-04 15:31:28 PDT
Created attachment 194869 [details]
biesi's review comments of attachment 193878 [details] [diff] [review]

r=biesi with these comments addressed
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-04 16:31:24 PDT
Comment on attachment 193878 [details] [diff] [review]
Ready to go, I think

the layout parts look fine. 

Well, better than fine :-)
Comment 38 Mikko Rantalainen 2005-09-05 01:00:38 PDT
This may be a stupid question, but do the following states share the same event
state number on purpose?

 #define NS_EVENT_STATE_VISITED      0x00000100
 
+// Content could not be rendered (image/object/etc).
+#define NS_EVENT_STATE_BROKEN       0x00000100
Comment 39 Boris Zbarsky [:bz] 2005-09-05 16:01:02 PDT
> but do the following states share the same event state number on purpose?

No, that was a mistake.  I fixed it but forgot to upload an updated diff...
Comment 40 Boris Zbarsky [:bz] 2005-09-05 20:07:47 PDT
Comment on attachment 194869 [details]
biesi's review comments of attachment 193878 [details] [diff] [review]

>Doesn't LoadImageWithChannel also need an UpdateState call?

Yes.  Added.

>Plugins? Maybe this should just say nsObjectFrame...

OK.

>So... if ImageState is called ImageState, should UpdateState be called
>UpdateImageState?

Done.

>When would a subclass call UpdateState? I.e. when would it change while not
>loading an image? (my point: maybe it should be private.)

It needs to be called by the AutoStateChanger helper struct, so that won't
work.

>AutoStateChange might be useful for ObjectLoadingContent too... maybe it
>should be a template? (would require not renaming ObjectState though, or
>giving the function as another template arg...)

I'd really rather do that sort of thing once both patches are in the tree, or
once one is in the tree and we're trying to update the other one to deal.

>Also... would it make sense to move the actual notification calling
>(AutoDocUpdate/ContentStatesChanged) to AutoStateChanger?

I actually moved in the opposite direction and switched to manual
ImageStateChanged in all but the one place where we actually have early
returns...  

>-  if (!mayNeedReframe) {
>
>OK, that is replaced by UpdateState, right?

Correct.  Or rather it's replaced by some code in the CSS frame constructor
that reframes on certain state changes.

>+    // No current request means error, since we weren't blocked
>
>REJECT_OTHER and REJECT_REQUEST don't count as blocked?

I've changed "blocked" to "disabled or suppressed".

>Is it OK to call ContentStateChanged with bits that didn't actually change?

More or less, but do I ever actually do that?

>(thinking of an <input> with a removed type="image" attribute while the
>image was loading. that should call onStopRequest eventually, including
>UpdateState)

I'm not sure I follow the example...

>Ah... ok, now I understand that mozBroken stuff :)
>So, why couldn't that property contain the flags directly?

Because it's a quick hack until your stuff lands?  ;)  If you really want I can
make this somehow better, but it really doesn't seem worthwhile.

>(huh, what's nsHTMLSharedObjectElement, other than "not built"?)

"an aborted attempt to do something".  I should probably file a bug to cvs
remove it.

>+    aContent->SetProperty(nsLayoutAtoms::imageFrame, NS_INT32_TO_PTR(1));
>
>Is this cheaper than GetPrimaryFrameFor+QI?

Probably not, but again it was just a quick hack.... GetPrimaryFor has the
usual "which presentation" problem, and for images it's not just presentation 0
that matters, so I figured I'd do things this way.

>+  // Note: we don't want to unser the broken property here, since that would
>
>unser -> unset?

Done
Comment 41 Boris Zbarsky [:bz] 2005-09-05 20:16:12 PDT
Created attachment 194979 [details] [diff] [review]
Apdated to comments
Comment 42 Boris Zbarsky [:bz] 2005-09-05 20:18:02 PDT
Created attachment 194980 [details] [diff] [review]
Same as diff -w
Comment 43 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-06 04:34:19 PDT
>>When would a subclass call UpdateState? I.e. when would it change while not
>>loading an image? (my point: maybe it should be private.)
>It needs to be called by the AutoStateChanger helper struct, so that won't
>work.

It's a friend...

>Is it OK to call ContentStateChanged with bits that didn't actually change?

More or less, but do I ever actually do that?

>>(thinking of an <input> with a removed type="image" attribute while the
>>image was loading. that should call onStopRequest eventually, including
>>UpdateState)
>I'm not sure I follow the example...

OK, so consider an <input type="image" src="...">. That image starts to load.
Then, removeAttribute("type") is called - the input is no longer an image,
CancelImageRequests is called, and this resets the state and calls
ContentStateChanged. BUT, the CancelImageRequests calls Cancel on the pending
request. This leads to an asynchronous OnStopDecode, and that one also calls
UpdateState.

This affects ImageState(), I think. But it doesn't affect IntrinsicState(),
which does not use the image state, being no longer an image. So the bits didn't
change.
Comment 44 Boris Zbarsky [:bz] 2005-09-06 09:45:46 PDT
> It's a friend...

Ah, ok.  Didn't realize friends could call private method.  I've now made
UpdateImageState private.

> This leads to an asynchronous OnStopDecode, and that one also calls
> UpdateState.

I don't believe the state changes here.  After CancelImageRequests, we have
mBroken == PR_TRUE and mBlocked == mUserDisabled == PR_FALSE (since the
mImageBlockingStatus is ACCEPT and mCurrentRequest is null).  But the same holds
when OnStopDecode is called, so there is no state change, and UpdateImageState
does check for an unchanged state.

That said, I can make the UpdateImageState call in OnStopDecode only if
mCurrentRequest is non-null, if you think that would be clearer.
Comment 45 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-06 10:41:33 PDT
> I don't believe the state changes here. 

Hm, I think it does: The first time, the <input> still thinks it's an image (hm,
though, does it?), and therefore it calls ImageState() in its IntrinsicState
calculation. The second time, it does no longer believe it's an image, does not
call ImageState(), and therefore loses the BROKEN state.
Comment 46 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-06 10:43:49 PDT
er, actually, the changed flags would be 0 in that case. hm...

>That said, I can make the UpdateImageState call in OnStopDecode only if
>mCurrentRequest is non-null, if you think that would be clearer.

I think that'd be better.

Hm... would onerror fire in the described case?
Comment 47 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-06 10:45:04 PDT
looking at that code again.. I think the assertion in OnStopDecode would fire too...
Comment 48 Boris Zbarsky [:bz] 2005-09-06 12:11:15 PDT
So I went ahead and checked.  Calling Cancel() on an imgIRequest means no more
notifications after that, since it sets mObserver to null.  So we don't have to
worry about OnStopDecode being called after Cancel(), in fact.
Comment 49 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-06 12:52:40 PDT
huh, ok... that's sorta different from every other nsIRequest impl, but ok...
Comment 50 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-18 09:49:41 PDT
I reviewed the style system changes and took a quick look at the others:

I'm not crazy about the out-of-order constants in nsIEventStateManager.
Instead, how about ending each line with comments like:
  // css3-selectors: ...
  // CSS2: ...
(where ... is optional)

Your nsCSSParser change needs to verify that it doesn't get
-moz-alt-content as something other than the first item in the list.
Currently it accepts 'open-quote -moz-alt-content', which doesn't seem
like what you intended since it doesn't accept '-moz-alt-content
open-quote'.

I'm a little concerned about adding large numbers of pseudo-element
selectors to the UA stylesheet since they don't benefit from the
RuleHash, but I guess we'll be ok.

Is there any good reason any of your UA stylesheet changes should *not*
be !important?

sr=dbaron with that (but I'd like to see an answer to the last question)
Comment 51 Boris Zbarsky [:bz] 2005-09-18 10:24:16 PDT
> Instead, how about ending each line with comments like:

Done.

> Your nsCSSParser change needs to verify

Good catch!  Done.

> Is there any good reason any of your UA stylesheet changes should *not*
> be !important?

I was thinking we should allow extensions or the browser UI to style these
elements any way they want to...  But they can still do that by using
nsIStyleSheetService and !important rules in the UA sheets they add, so you're
right -- there is no good reason not to make these !important.  I'll make that
change.
Comment 52 Boris Zbarsky [:bz] 2005-09-18 10:56:47 PDT
Created attachment 196569 [details] [diff] [review]
Updated to dbaron's comments
Comment 53 Boris Zbarsky [:bz] 2005-09-18 12:47:32 PDT
Created attachment 196577 [details] [diff] [review]
Additional patch to fix Tp regression

I knew I'd forgotten something...  This makes sure we don't bother notifying of
content state changes on initial insertion into the document.  I'm hoping said
notifications were what cased the 10% jump in Tp... ;)
Comment 54 Boris Zbarsky [:bz] 2005-09-18 14:05:59 PDT
Created attachment 196589 [details] [diff] [review]
Additional additional patch to fix Tp regression some more

I hate imagelib.  :(
Comment 55 Boris Zbarsky [:bz] 2005-09-18 22:12:55 PDT
Fixed, and Tp is happy.  This seems to cause some issues with adblock -- bug 309076.
Comment 56 mv_van_rantwijk 2005-09-29 05:40:13 PDT
A part of this patch (display: none !important;) breaks the layout of My Yahoo!

People expect the buttons on a certain location, but that no longer applies in
builds with this patch. 

For example: the sign out link/button is normally positioned in the middle of
the page, at least before this patch went in, but now it is at the start/left
side of the page (when you block some of the images on that page).

I'm not the only one blocking images, so this is probably getting (very)
annoying for others a well. 

Is there a work around that can be used in userChrome.css until this issue is
solved?
Comment 57 Boris Zbarsky [:bz] 2005-09-29 07:19:29 PDT
Please file bugs on issues this patch caused.  One bug per issue, blocking this
bug.  There's no way we're going to discuss all the issues in this bug, so...
Comment 58 mv_van_rantwijk 2005-09-29 07:35:07 PDT
(In reply to comment #57)
> Please file bugs on issues this patch caused.  One bug per issue, blocking this
> bug.  There's no way we're going to discuss all the issues in this bug, so...

-> bug 310447
Comment 59 Biju 2005-10-07 01:15:42 PDT
It will be very helpful if somebody can write an example showing how to 
:-moz-alt-text, :-moz-placeholder, :-moz-broken and any other newly added
pseudo-classes/attribute
Comment 60 Eric Shepherd [:sheppy] 2007-10-02 08:13:45 PDT
Yes, an example showing how these work would be very helpful.
Comment 61 Boris Zbarsky [:bz] 2007-10-02 12:33:33 PDT
The documentation at http://lxr.mozilla.org/seamonkey/source/content/events/public/nsIEventStateManager.h#206 basically describes what the pseudo-classes match, for what it's worth.
Comment 62 Eric Shepherd [:sheppy] 2008-06-02 09:17:01 PDT
docs are done here.
Comment 63 Boris Zbarsky [:bz] 2010-08-27 11:34:08 PDT
The docs were apparently based on comment 21, not on what was actually implemented and what I linked to in comment 61.  Probably my fault for not linking to the actual names of the pseudo-classes too.
Comment 64 Eric Shepherd [:sheppy] 2010-08-27 11:56:07 PDT
The reason this was documented wrong is that nobody updated the summary to list the updated names for these, so I went off the descriptions of those names as found in the comments.

On top of that, I asked questions about :-moz-placeholder, by that name, and actually got answers that resulted in the docs as they stand now, instead of being told that name wasn't correct. :)

I'm working on correcting the docs now.
Comment 65 Boris Zbarsky [:bz] 2010-08-27 12:14:08 PDT
Yeah, like I said, my fault.  The pseudo-classes we implemented are:

142 // Image, object, etc state pseudo-classes
143 CSS_STATE_PSEUDO_CLASS(mozBroken, ":-moz-broken", NS_EVENT_STATE_BROKEN)
144 CSS_STATE_PSEUDO_CLASS(mozUserDisabled, ":-moz-user-disabled",
145                        NS_EVENT_STATE_USERDISABLED)
146 CSS_STATE_PSEUDO_CLASS(mozSuppressed, ":-moz-suppressed",
147                        NS_EVENT_STATE_SUPPRESSED)

which are documented next to the corresponding NS_EVENT_* constants in nsIEventStateManager.

Since then we have also added:

148 CSS_STATE_PSEUDO_CLASS(mozLoading, ":-moz-loading", NS_EVENT_STATE_LOADING)
149 CSS_STATE_PSEUDO_CLASS(mozTypeUnsupported, ":-moz-type-unsupported",
150                        NS_EVENT_STATE_TYPE_UNSUPPORTED)
151 CSS_STATE_PSEUDO_CLASS(mozHandlerDisabled, ":-moz-handler-disabled",
152                        NS_EVENT_STATE_HANDLER_DISABLED)
153 CSS_STATE_PSEUDO_CLASS(mozHandlerBlocked, ":-moz-handler-blocked",
154                        NS_EVENT_STATE_HANDLER_BLOCKED)
155 CSS_STATE_PSEUDO_CLASS(mozHandlerCrashed, ":-moz-handler-crashed",
156                        NS_EVENT_STATE_HANDLER_CRASHED)

in various Gecko versions; these might also be worth documenting.  Please ask any questions that come up, either here or in e-mail?

Note You need to log in before you can comment on or make changes to this bug.