Closed Bug 11011 (moz-broken) Opened 25 years ago Closed 19 years ago

Need pseudo-classes for when alt text is shown (:-moz-alt-text, :-moz-placeholder, :-moz-broken)

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: MatsPalmgren_bugz, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [Hixie-P5] (but blocking some [Hixie-P3] bugs...))

Attachments

(5 files, 5 obsolete files)

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
Attached file testcase —
Whiteboard: [TESTCASE]
Assignee: troy → kipp
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
Cc'ing David as well, because he probably knows as well
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.)
Status: NEW → ASSIGNED
Target Milestone: M12
*** Bug 10280 has been marked as a duplicate of this bug. ***
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.
Summary: ALT texts for <IMG> elements is overlapping → Need a pseudo-class for when alt text is shown
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.
QA Contact: petersen → py8ieh=bugzilla
As agreed with Beth, assigning myself as QA contact for all 'alt text' bugs...
Target Milestone: M12 → M15
Assignee: kipp → troy
Status: ASSIGNED → NEW
Hey troy - can you either WONTFIX this because you don't care, or do what they
suggest...it sounds reasonable to me...thanks
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → LATER
It is reasonable, but we're not doing it for the first version
Status: RESOLVED → VERIFIED
Based on Troy's comments, verifying. I'll reopen this after we release 1.0.
Summary: Need a pseudo-class for when alt text is shown → {css-moz} Need a pseudo-class for when alt text is shown
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?
Blocks: 8131
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
> 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.
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.
Keywords: css-moz
This is likely to be specified by CSS3.
Keywords: css3
Summary: {css-moz} Need a pseudo-class for when alt text is shown → Need a pseudo-class for when alt text is shown
Blocks: 34981
Moving to future...
Status: VERIFIED → REOPENED
Resolution: LATER → ---
Whiteboard: [TESTCASE]
Target Milestone: M15 → Future
Reassigning to Buster...
Assignee: troy → buster
Status: REOPENED → NEW
Depends on: 1994
Blocks: 23691
Summary: Need a pseudo-class for when alt text is shown → Need a pseudo-class for when alt text is shown [INLINE]
Keywords: testcase
*** Bug 65206 has been marked as a duplicate of this bug. ***
Depends on: 75185
No longer depends on: 1994
Whiteboard: [Hixie-P5] (but blocking some [Hixie-P3] bugs...)
No longer blocks: alttext
Build reassigning Buster's bugs to Marc.
Assignee: buster → attinasi
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.
Summary: Need a pseudo-class for when alt text is shown [INLINE] → Need pseudo-classes for when alt text is shown [INLINE] :-moz-alt-text :-moz-placeholder
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....
Depends on: 83774
Blocks: 180622
Need for new pseudo-classes -> style system.

Assignee: attinasi → dbaron
Component: Layout → Style System
Keywords: mozilla1.1
Depends on: 191839
Summary: Need pseudo-classes for when alt text is shown [INLINE] :-moz-alt-text :-moz-placeholder → Need pseudo-classes for when alt text is shown (:-moz-alt-text, :-moz-placeholder)
Blocks: 12770
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).
Attached patch First cut at a patch (obsolete) — — Splinter Review
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...
Assignee: dbaron → bzbarsky
Status: NEW → ASSIGNED
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.
Blocks: 114641
Priority: P3 → P1
Target Milestone: Future → mozilla1.9alpha
Please make sure you are familiar with http://www.hixie.ch/specs/alttext .
> 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?
> 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.)
Blocks: 288064
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.
> 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).
Depends on: 296309
Summary: Need pseudo-classes for when alt text is shown (:-moz-alt-text, :-moz-placeholder) → Need pseudo-classes for when alt text is shown (:-moz-alt-text, :-moz-placeholder, :-moz-broken)
Blocks: 301638
Blocks: 304598
Attached patch Ready to go, I think (obsolete) — — Splinter Review
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.
Attachment #182840 - Attachment is obsolete: true
Attachment #193878 - Flags: superreview?(dbaron)
Attachment #193878 - Flags: review?(roc)
Attachment #193878 - Flags: review?(cbiesinger)
Attachment #193878 - Flags: approval1.8b4-
Attached patch Same thing as diff -w (obsolete) — — Splinter Review
This makes some of the nsCSSFrameConstructor::ConstructHTMLFrame changes much
simpler to review.
Attachment #193878 - Flags: approval1.8b4-
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?
> 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.  ;)
r=biesi with these comments addressed
Attachment #193878 - Flags: review?(cbiesinger) → review+
Comment on attachment 193878 [details] [diff] [review]
Ready to go, I think

the layout parts look fine. 

Well, better than fine :-)
Attachment #193878 - Flags: review?(roc) → review+
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
> 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 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
Attached patch Apdated to comments (obsolete) — — Splinter Review
Attachment #193878 - Attachment is obsolete: true
Attachment #193880 - Attachment is obsolete: true
Attached patch Same as diff -w (obsolete) — — Splinter Review
Attachment #194979 - Flags: superreview?(dbaron)
Attachment #194979 - Flags: review?
Attachment #193878 - Flags: superreview?(dbaron)
>>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.
> 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.
> 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.
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?
looking at that code again.. I think the assertion in OnStopDecode would fire too...
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.
huh, ok... that's sorta different from every other nsIRequest impl, but ok...
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)
> 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.
Attachment #194979 - Flags: superreview?(dbaron) → superreview+
Attached patch Updated to dbaron's comments — — Splinter Review
Attachment #194979 - Attachment is obsolete: true
Attachment #194980 - Attachment is obsolete: true
Attachment #194979 - Attachment is obsolete: false
Attachment #194979 - Flags: review?
Blocks: 309065
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... ;)
Attachment #194979 - Attachment is obsolete: true
Attachment #196577 - Flags: superreview?(roc)
Attachment #196577 - Flags: review?(cbiesinger)
Attachment #196577 - Flags: review?(cbiesinger) → review+
I hate imagelib.  :(
Attachment #196589 - Flags: superreview?(roc)
Attachment #196589 - Flags: review?(cbiesinger)
No longer blocks: 309076
Depends on: 309076
Attachment #196589 - Flags: superreview?(roc) → superreview+
Attachment #196589 - Flags: review?(cbiesinger) → review+
Fixed, and Tp is happy.  This seems to cause some issues with adblock -- bug 309076.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago19 years ago
Resolution: --- → FIXED
Depends on: 309141
Depends on: 309237
Blocks: 71473
Blocks: 70820
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?
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...
Depends on: 310447
(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
Depends on: 310662
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
Depends on: 315541
Blocks: 268157
Blocks: 285212
Depends on: 329896
Keywords: dev-doc-needed
Depends on: 380144
Yes, an example showing how these work would be very helpful.
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.
docs are done here.
Blocks: 395390
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.
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.
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?
Hi,

I don't know if this is the best place to ask, but I was hoping to replace the loading and broken image placeholders with my own in a theme.  Is there any way their location can be defined?  I tried to get the pseudo-classes mentioned above to do something like that, but with no success.
Do let me know how/if this is possible.

Thanks!
The placeholders are loaded from resource://gre-resources/loading-image.png and resource://gre-resources/broken-image.png but I'm not sure a theme can easily override what those do.

You can sort of hack around things by styling loading images as "width: auto; height: auto" and setting the "browser.display.force_inline_alttext" preference to true, then using ::before or whatnot to pull in your own image.  But this will have some performance penalty (e.g. will force a relayout when an image goes from "loading" to "loaded") and will interfere a bit with usability...

The best solution here might just be to use some sort of skinnable/themable URLs for the two placeholder images.  Separate bug on that, please?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: