Closed Bug 188734 Opened 18 years ago Closed 17 years ago

implement :target


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






(Reporter: dbaron, Assigned: dbaron)




(Keywords: css3, testcase, Whiteboard: [patch])


(5 files, 1 obsolete file)

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.
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 ,
including when going back/forward through history.
Priority: -- → P3
Whiteboard: [patch]
Target Milestone: --- → mozilla1.3beta
Comment on attachment 111319 [details] [diff] [review]

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 !!!
Attachment #111319 - Flags: review+
Attachment #111319 - Flags: superreview?(bzbarsky)
Comment on attachment 111319 [details] [diff] [review]

     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.
Attachment #111319 - Flags: superreview?(bzbarsky) → superreview+
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.
Yeah, add a comment explaining that.  ;)
Attached file 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.
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 on attachment 111319 [details] [diff] [review]

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).
oops, slight logic error in previous version
Attachment #111440 - Attachment is obsolete: true
Attachment #111442 - Flags: superreview?(bzbarsky)
Attachment #111442 - Flags: review?(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").
Attachment #111442 - Flags: superreview?(bzbarsky)
Attachment #111442 - Flags: superreview+
Attachment #111442 - Flags: review?(bzbarsky)
Attachment #111442 - Flags: review+
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
Comment on attachment 111442 [details] [diff] [review]
patch to fix ContentStatesChanged

This patch caused bug 189292, which I just fixed.
Summary: implement CSS3 :target pseudo-class → implement :target (left open for ContentStatesChanged bug)
Summary: implement :target (left open for ContentStatesChanged bug) → implement :target (left open for :target on root)
per WG, if there is no fragment identifier, the :target pseudo-class matches the
root element.
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{

So the first time you visit the page, the content of "general" should be visible.
Blocks: selectors3
Summary: implement :target (left open for :target on root) → implement :target (left open for :target on :root)
Summary: implement :target (left open for :target on :root) → implement :target (left open for :target on root)
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.
AFAIK that is bug 15608, please don't mix up bugs.
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.
Keywords: testcase
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.
note to self (confidential):

Marking FIXED since this has been fixed for quite a while.
Closed: 17 years ago
Resolution: --- → FIXED
Summary: implement :target (left open for :target on root) → implement :target
And now the test case is valid again.
You need to log in before you can comment on or make changes to this bug.