188734 - implement :target

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[David Baron :dbaron:]

I was just studying for my exams, and procrastinating by thinking about Daniel's
blog entry about how neat :target was, when I realized that the simple way to
implement :target would be as an event state, since that would give us much of
the code needed to handle dynamic changes for free.

So I just wrote the patch.  I'll attach it here soon assuming it works correctly.

Comment 1

[David Baron :dbaron:]

Attachment: patch

Comment 2

[David Baron :dbaron:]

This might have some issues with the case where the dynamic changes caused by
the stylechange cause a reflow, in that it would scroll to the old location of
the anchor rather than the new one (or that it might get confused if there were
no frame for the anchor before the dynamic change).  It's worth writing
testcases for that.

However, this passes the test at
http://www.geocities.com/glazou_2000/css3-modsel-tests/css3-modsel-48.xml ,
including when going back/forward through history.

Comment 3

[David Baron :dbaron:]

The blog entry I mentioned in comment 0 is
http://daniel.glazman.free.fr/weblog/archived/2003_01_05_glazblogarc.html#87183885

Comment 4

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

Comment on attachment 111319 [details] [diff] [review]
patch

wow!!! i had a patch myself for this but its dynamic part was not totally
effective. (a) make it land asap (b) don't call it -moz-target !!!

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111319 [details] [diff] [review]
patch

     if ((NS_LossyConvertUCS2toASCII(aAnchorName).EqualsIgnoreCase("top")) &&
+	 (compatMode == eCompatibility_NavQuirks)) {
+      rv = NS_OK;
+      if (aScroll && mViewManager) {

I almost think the code would be clearer if the aScroll && mViewManager check
came first, with the mode-getting code and the string comparison moved inside
it... But either way is fine.

Comment 6

[David Baron :dbaron:]

I should probably add a comment for the reason for doing it the rather odd way
that I did -- I want to ensure that the return value is the same whether or not
|aScroll| is given, since there is code on the caller end that tries the same
thing in different encodings, given a failure return value.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, add a comment explaining that.  ;)

Comment 8

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

Attachment: test case

David, you should try this test case I just wrote. It does not fully work.
First click on the Mozilla tab, it displays the corresponding tabpanel. But
then click on another tab, it does not display the tabpanel until you reclick
the anchor.

Comment 9

[David Baron :dbaron:]

I've poked into the problems with Daniel's testcase, and it seems related to
general problems with nsCSSFrameConstructor::ContentStateChanged and not a
problem with this patch.  (In particular, note that the transitions that fail
have primaryFrame1 = null but primaryFrame2 != null, so we go into the |else if
(primaryFrame2)| rather than the else following.

Comment 10

[David Baron :dbaron:]

Comment on attachment 111319 [details] [diff] [review]
patch

Checked in this patch (with added comment) to the trunk, 2003-01-13 15:10 PDT.

Leaving this bug open for ContentStatesChanged fix and potentially for making
:target apply to the root when there's no fragment identifier (depending on WG
decision -- the proposal makes sense to me and seems useful).

Comment 11

[David Baron :dbaron:]

Attachment: patch to fix ContentStatesChanged

This fixes attachment 111383 [details].

Comment 12

[David Baron :dbaron:]

Attachment: patch to fix ContentStatesChanged

oops, slight logic error in previous version

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111442 [details] [diff] [review]
patch to fix ContentStatesChanged

Looks good, though the error-handling in this function is kinda wacky (eg see
the last two if statements; both just assign to "result").

Comment 14

[David Baron :dbaron:]

Comment on attachment 111442 [details] [diff] [review]
patch to fix ContentStatesChanged

Checked in to trunk, 2003-01-13 17:29 PDT (with one additional tweak, since I
noticed one of the three theme sections was missing the ThemeSupportsWidget
call).

Comment 15

[David Baron :dbaron:]

Comment on attachment 111442 [details] [diff] [review]
patch to fix ContentStatesChanged

This patch caused bug 189292, which I just fixed.

Comment 16

[Hixie (not reading bugmail)]

per WG, if there is no fragment identifier, the :target pseudo-class matches the
root element.

Comment 17

[Anne (:annevk)]

Attachment: test case [:target matches the root element]

Comment 18

[Anne (:annevk)]

Added myself to the cc list.

About the test case: "test case [:target matches the root element]". The first
time you visit the page there is no fragment identifier, so :target matches
root. I made this rule to test that:

:target div#general{
 display:block;
}

So the first time you visit the page, the content of "general" should be visible.

Comment 19

[James Ascroft-Leigh]

Also procrastinating; I am styling the HTML output from my coursework after XSL
from DocBook when I really should be writing the document.  The glossary part of
this single HTML file is made up of a <dl/> containing <dt/>-<dd/> pairs for
each glossary entry.  The id attribute is on the <dt/> but I would like to style
the corresponding <dd/> as well.

I used the CSS:
DT:target, DT:target + DD { background-color: #lime; }

That does not seem to work.  I think it would be nice if it did and, in fact,
when I entered the code into the EditCSS [1] sidebar for Mozilla Firebird it did
work in some cases.  I did not seem very predictable, I will investigate the
exact behaviour later if you like.

I have prepared a simple example document with this structure and style that I
will attach shortly.

Comment 20

[James Ascroft-Leigh]

Attachment: Test case for selectors using the :target bt not as the subject

Comment 21

[Anne (:annevk)]

AFAIK that is bug 15608, please don't mix up bugs.

Comment 22

[Arnoud Berendsen]

There is a problem with margin-bottom being applied twice when using :target in
combination with generated content. I thought it too specialistic for this bug
so I filed bug 229674 on this, but you might want to take a look.

Comment 23

[David Baron :dbaron:]

Comment on attachment 111442 [details] [diff] [review]
patch to fix ContentStatesChanged

>+      // Detect if one is the ancestor of the other, and skip if so.
>+      if (aContent1 && aContent2) {
>+        if (aContent1 == aContent2)
>+          aContent2 = nsnull;
>+        else if (IsAncestorOf(aContent1, aContent2))
>+          aContent2 = nsnull;
>+        else if (IsAncestorOf(aContent2, aContent1)) {
>+          aContent1 = nsnull;
>+        }
>+      }

This code was wrong (since it comes before the HasStateDependentStyle test),
and I removed it while fixing bug 15608.

Comment 24

[David Baron :dbaron:]

note to self (confidential):
http://lists.w3.org/Archives/Member/w3c-css-wg/2003JulSep/0359.html

Marking FIXED since this has been fixed for quite a while.

Comment 25

[Anne (:annevk)]

Does this mean that
<http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/xml/full/flat/css3-modsel-21c.xml>
is an invalid test case?

Comment 26

[Hixie (not reading bugmail)]

Yes.

Comment 27

[Anne (:annevk)]

And now the test case is valid again.