b4e78d426dcdb47a20f1979025d5a0969ea79ac4 braney Thu Sep 17 17:07:49 2026 -0700 docent: a box: check for where an element sits, and lists for text:/noText:, refs #37892 `expect:` could say what was in a page and never where it was on the screen. has:/noHas: take a CSS selector, which describes the tree; color: reads pixels but only inside a track's row. #38251 moved the narrow-window menu icon out of the blue bar with every selector still matching and every word of the page still there, and nothing in the language could ask about it. box: takes `inside:` (every edge within another element's box, with `tolerance:` px of slack), `clear:` (no overlap with anything a selector matches, `gap:` for a minimum separation) and `height:`/`width:`. Every element `sel:` matches has to satisfy every clause, so {sel: "ul.nice-menu > li", inside: "#main-menu-whole"} reads as "every menu item is in the bar". Boxes are read in document coordinates; an element with no box at all is skipped rather than treated as a zero-sized box at the origin, which would sit "inside" anything. A failure prints the measurement: #topRightLinks is not inside #main-menu-whole: 114px above it -- it is at 666,0 34x32, #main-menu-whole at 0,114 1000x32 The image height: and box's own height:/width: now share one cmpSize(), so the two cannot drift into different comparison grammars. text: and noText: take a list, the way rows:, has: and noHas: always did. They have to: a list handed to a check that stringifies its argument fails OPEN -- ["a", "b"] becomes "a,b", which no page contains, so it passes on anything, and passes silently. Six scripts in one batch were written that way and all six looked green. pagechecks and its .xfail twin cover both, the xfail with one entry aimed wrongly and one aimed rightly in each list, so it fails only if every entry is really looked at on its own. diff --git src/hg/utils/docent/docent.js src/hg/utils/docent/docent.js index 626bbc0c08f..e47fe591f47 100755 --- src/hg/utils/docent/docent.js +++ src/hg/utils/docent/docent.js @@ -1009,30 +1009,140 @@ const n = new Map(); let total = 0; for (let i = 0; i < d.length; i += 4) { const r = d[i], gg = d[i + 1], b = d[i + 2], a = d[i + 3]; if (a < 8) continue; // nothing drawn here if (r >= 250 && gg >= 250 && b >= 250) continue; // background const k = r + ',' + gg + ',' + b; n.set(k, (n.get(k) || 0) + 1); total++; } const top = [...n.entries()].sort((p, q) => q[1] - p[1]).slice(0, 6) .map(([k, v]) => ({ c: k.split(',').map(Number), n: v })); return { top, total, box: [xa, xb, w, h] }; }, { id, frac, xpx: (o.x != null) ? Number(o.x) : null, wide: Number(o.wide || 5) }); } + // A size against a comparison spec: a bare number is a CEILING, which is the check anyone + // actually wants, and "<1200", ">=300", "=850" are there when it is not. Shared by the + // image `height:` and by `box:`, so the two cannot drift into different grammars. + // Returns null when it holds, or the text of what went wrong. + function cmpSize(got, spec, what) { + const m = /^\s*(<=|>=|<|>|=)?\s*(\d+)\s*$/.exec(String(spec)); + if (!m) return `${what}: cannot read "${spec}"`; + const n = Number(m[2]), op = m[1] || '<='; + const ok = op === '<' ? got < n : op === '>' ? got > n + : op === '>=' ? got >= n : op === '=' ? got === n : got <= n; + return ok ? null : `${what} is ${Math.round(got)}px, wanted ${op}${n}`; + } + // The bounding boxes of everything a selector matches, in DOCUMENT coordinates (the page's + // own scroll added in), so two elements can be compared even when the page has been + // scrolled between the two reads. An element with no box at all -- display:none, or never + // laid out -- is dropped rather than reported as a zero-sized box at the origin, which + // would sit "inside" anything. + async function elemRects(sel) { + return await page.evaluate(s => { + const out = []; + document.querySelectorAll(s).forEach(e => { + const r = e.getBoundingClientRect(); + if (r.width <= 0 && r.height <= 0) return; + out.push({ left: r.left + scrollX, top: r.top + scrollY, + right: r.right + scrollX, bottom: r.bottom + scrollY, + width: r.width, height: r.height }); + }); + return out; + }, sel).catch(() => null); + } + const showRect = r => `${Math.round(r.left)},${Math.round(r.top)} ` + + `${Math.round(r.width)}x${Math.round(r.height)}`; + // box: WHERE an element sits, which no other check here can ask. has:/noHas: say what is in + // the page and never where it is, and color: samples a track row inside the image and + // cannot be pointed at a page element. #38251 is the bug that needs this: the narrow-window + // menu icon left the blue bar and slid across the menu items, with the same elements, the + // same text and the same selectors matching either way. + // + // box: {sel: "#topRightLinks", inside: "#main-menu-whole"} every edge within that box + // box: {sel: "#topRightLinks", clear: "ul.nice-menu > li > a", gap: 8} overlaps none + // box: {sel: "#main-menu-whole", height: "<=34", width: ">=1000"} + // + // Name the LINK rather than the list item when asking about overlap: an li's box carries + // padding and is wider than the label inside it, so a clear: on the li can fail for a + // reason that is not a bug. + // + // EVERY element `sel:` matches has to satisfy every clause, which is the natural reading of + // `{sel: "ul.nice-menu > li", inside: "#main-menu-whole"}` -- every menu item is in the bar + // -- and behaves the way a first-match rule would when the selector names one element. + // `inside:` and `clear:` take the first match of THEIR selector, since a container is one + // element, except that `clear:` is checked against all of them: "it must not cover any menu + // item" is the question being asked. + // + // `tolerance:` (default 1px) is slack on `inside:`, for a border or a rounded edge that + // rounds the wrong way. `gap:` on `clear:` asks for that many pixels of clear space rather + // than merely for no overlap. + async function boxCheck(o) { + const bad = []; + if (!o.sel) return ['box: needs sel:']; + const mine = await elemRects(o.sel); + if (mine == null) return [`box: cannot read the selector "${o.sel}"`]; + if (!mine.length) return [`box: nothing with a box matches "${o.sel}"`]; + const tol = (o.tolerance != null) ? Number(o.tolerance) : 1; + // Which of several matches went wrong is worth saying, and saying nothing when there is + // only one: "ul.nice-menu > li (3 of 9)" reads badly for a single #topRightLinks. + const which = i => mine.length > 1 ? `${o.sel} (${i + 1} of ${mine.length})` : o.sel; + let outer = null, others = null; + if (o.inside != null) { + outer = await elemRects(o.inside); + if (outer == null) bad.push(`box: cannot read the selector "${o.inside}"`); + else if (!outer.length) bad.push(`box: nothing with a box matches "${o.inside}"`); + } + if (o.clear != null) { + others = await elemRects(o.clear); + if (others == null) bad.push(`box: cannot read the selector "${o.clear}"`); + else if (!others.length) bad.push(`box: nothing with a box matches "${o.clear}"`); + } + const gap = Number(o.gap || 0); + mine.forEach((me, i) => { + if (outer && outer.length) { + const b = outer[0]; + const off = []; + if (me.left < b.left - tol) off.push(`${Math.round(b.left - me.left)}px past its left`); + if (me.right > b.right + tol) off.push(`${Math.round(me.right - b.right)}px past its right`); + if (me.top < b.top - tol) off.push(`${Math.round(b.top - me.top)}px above it`); + if (me.bottom > b.bottom + tol) off.push(`${Math.round(me.bottom - b.bottom)}px below it`); + if (off.length) + bad.push(`${which(i)} is not inside ${o.inside}: ${off.join(', ')}` + + ` -- it is at ${showRect(me)}, ${o.inside} at ${showRect(b)}`); + } + if (others && others.length) { + // A selector can legitimately match the element under test as well (every li, say, + // against every li); an element never has to be clear of itself. + const hits = others.filter(r => !(r.left === me.left && r.top === me.top + && r.width === me.width && r.height === me.height) + && me.left < r.right + gap && r.left < me.right + gap + && me.top < r.bottom + gap && r.top < me.bottom + gap); + if (hits.length) + bad.push(`${which(i)} at ${showRect(me)} is not clear of ${hits.length} of ` + + `${others.length} "${o.clear}"` + + (gap ? ` by ${gap}px` : '') + `: ${hits.slice(0, 3).map(showRect).join('; ')}`); + } + for (const [k, spec] of [['height', o.height], ['width', o.width]]) { + if (spec == null) continue; + const why = cmpSize(me[k], spec, `${which(i)} ${k}`); + if (why) bad.push(why); + } + }); + return bad; + } // "r,g,b" or "#rrggbb" -> [r,g,b]. No color NAMES on purpose: trackDb's `color 0,255,0` // is not CSS `green` (#008000), and a script that says one and means the other would be // wrong in a way nobody would look for. function parseRgb(v) { const s = String(v).trim(); let m = /^#?([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})$/i.exec(s); if (m) return [1, 2, 3].map(i => parseInt(m[i], 16)); m = /^(\d{1,3})\s*,\s*(\d{1,3})\s*,\s*(\d{1,3})$/.exec(s); if (m) { const v3 = [1, 2, 3].map(i => Number(m[i])); return v3.every(x => x <= 255) ? v3 : null; } return null; } // Hover an item to raise its mouseover tooltip (real mousemove -> the browser's own // tooltip). Two ways to place the cursor: // IDENTITY `item:` / `title:` / `value:` -> name the item (lands on the right ROW). // POSITION `at:` (genomic coord) / `frac:` (0..1) / `x:` (raw px) -> a point. @@ -1409,47 +1519,57 @@ // that never hid, a pinned tooltip that grabbed the neighbouring item, an Apache 414 // page where the view should be. All of those shipped once and all were caught by eye. // Stating the expectation instead stops the run, non-zero, at the step that broke it -- // `make` then fails rather than writing a wrong figure over a right one. // // expect: {rows: [ruler, mane]} these rows were drawn // expect: {rows: [ruler, mane], exact: true} ... and nothing else // expect: {rows: [ruler, mane], ordered: true} ... in that order, top to bottom // expect: {noRows: [clinvarCnv]} this row was not // expect: {height: 2000} the still is no taller than this ("<1200" etc.) // expect: {tip: "mismatch A->C"} the tooltip now up says this // expect: {text: "...", noText: "..."} the page does / does not contain this // expect: {url: "hgSearch", noUrl: "%E2%80%8B"} the address bar does / does not // expect: {has: "#td_data_mane map[name=map_center_mane]"} this selector matches // expect: {noHas: "#td_data_knownGene map[name=map_center_mane]"} ... does not + // expect: {box: {sel: "#topRightLinks", inside: "#main-menu-whole"}} where it sits // expect: {color: {track: crm4, is: "0,0,255"}} the items in that row are drawn blue // expect: {color: {track: crm4, part: label, is: "0,255,0"}} ... its center label green // + // `text:`, `noText:`, `has:`, `noHas:`, `rows:` and `noRows:` all take one value or a LIST + // of them. That matters most for the two text checks: a check that stringifies its argument + // turns ["a", "b"] into "a,b", which no page contains, so it would pass on anything -- and + // pass silently, which is worse than failing. + // // `url:`/`noUrl:` are a substring check on the CURRENT address, which is the only place // some things are visible at all: which CGI a click actually reached, and what the page // put in a query string. #36387's fix strips zero-width characters out of a search term // before the position box submits it, and the term is invisible in the rendered page -- // the only evidence either way is whether `%E2%80%8B` survives into the URL. // - // `has:`/`noHas:` are for a bug whose whole signature is WHERE something sits in the - // page. #37785 attached a squishyPack track's center label to the wrong row: same rows - // drawn, same total height, same pixels -- only the row the label hangs off changed, so - // rows:, height: and text: are all blind to it. Both take a CSS selector, or a list of + // `has:`/`noHas:` are for a bug whose whole signature is where something sits in the + // page's TREE. #37785 attached a squishyPack track's center label to the wrong row: same + // rows drawn, same total height, same pixels -- only the row the label hangs off changed, + // so rows:, height: and text: are all blind to it. Both take a CSS selector, or a list of // them, and each may name several elements. Reach for these last: an assertion on // hgTracks' own ids and classes is the most likely thing here to break for a reason // that is not a bug. // + // `box:` is for where something sits on the SCREEN, which a selector cannot say at all. + // #38251's icon left the blue bar and slid across the menu items at narrow widths with + // every selector still matching. See boxCheck(). + // // `color:` is the one check that reads the IMAGE rather than the page, because a bug about // color changes nothing else: same rows, same height, same items, same tooltips. It names // the color the row is mostly drawn in (`is:`) or the one it must not be (`not:`), and // `part: label` asks about the center label instead of the items. See rowColors(). // // `warn: true` downgrades a failure to a warning, for a check worth logging but not worth // stopping a build over. async function expectState(arg) { const o = (arg && typeof arg === 'object') ? arg : { text: arg }; const url = page.url(); const seen = await page.evaluate(() => { const im = document.getElementById('imgTbl'); const tip = document.getElementById('mouseoverContainer'); const up = tip && tip.offsetWidth > 0 && getComputedStyle(tip).display !== 'none' && getComputedStyle(tip).visibility !== 'hidden'; @@ -1483,61 +1603,62 @@ // second failure saying the same thing. Without this a set test cannot fail for a // wrong order, which is the whole of a bug like a quickLift target coming back in // request order rather than source order (#38032). if (o.ordered) { const seq = want.map(w => ({ w, at: seen.rows.findIndex(r => r === w || r.endsWith('_' + w)) })) .filter(e => e.at >= 0); for (let i = 1; i < seq.length; i++) if (seq[i].at < seq[i - 1].at) { bad.push(`rows out of order: ${seq[i - 1].w} should be drawn above ${seq[i].w}`); break; } } const banned = list(o.noRows).filter(w => drawn(w)); if (banned.length) bad.push(`rows that should not be drawn: ${banned.join(', ')}`); if (o.height != null) { - // A bare number is a ceiling, which is the check anyone actually wants. - const m = /^\s*(<=|>=|<|>|=)?\s*(\d+)\s*$/.exec(String(o.height)); - if (!m) bad.push(`height: cannot read "${o.height}"`); - else { - const n = Number(m[2]), op = m[1] || '<='; - const ok = op === '<' ? height < n : op === '>' ? height > n - : op === '>=' ? height >= n : op === '=' ? height === n : height <= n; - if (!ok) bad.push(`image is ${height}px, wanted ${op}${n}`); - } + const why = cmpSize(height, o.height, 'image'); + if (why) bad.push(why); } if (o.tip != null && !seen.tip.includes(String(o.tip))) bad.push(seen.tip ? `tooltip says "${seen.tip}", wanted "${o.tip}"` : `no tooltip is up, wanted "${o.tip}"`); - if (o.text != null && !seen.text.includes(String(o.text))) - bad.push(`page does not contain "${o.text}"`); - if (o.noText != null && seen.text.includes(String(o.noText))) - bad.push(`page contains "${o.noText}"`); + // text:/noText: take one string or a LIST of them, the way has:/noHas: do. They have to: + // a list handed to a check that stringifies its argument fails OPEN -- ["a", "b"] becomes + // "a,b", which no page contains, so the check passes on anything and passes silently. + // Six scripts in one batch were written that way and all six looked green. + for (const want of list(o.text)) + if (!seen.text.includes(want)) bad.push(`page does not contain "${want}"`); + for (const want of list(o.noText)) + if (seen.text.includes(want)) bad.push(`page contains "${want}"`); if (o.url != null && !url.includes(String(o.url))) bad.push(`url is "${url}", wanted it to contain "${o.url}"`); if (o.noUrl != null && url.includes(String(o.noUrl))) bad.push(`url contains "${o.noUrl}": ${url}`); for (const sel of list(o.has)) { const n = await page.locator(sel).count().catch(() => -1); if (n === 0) bad.push(`nothing matches "${sel}"`); else if (n < 0) bad.push(`has: cannot read the selector "${sel}"`); } for (const sel of list(o.noHas)) { const n = await page.locator(sel).count().catch(() => -1); if (n > 0) bad.push(`${n} element(s) match "${sel}", wanted none`); else if (n < 0) bad.push(`noHas: cannot read the selector "${sel}"`); } + // box: where an element sits. A list is allowed and every entry is checked, so one step + // can state a whole layout and a failure names every part of it that came out wrong. + for (const one of (o.box == null ? [] : (Array.isArray(o.box) ? o.box : [o.box]))) + bad.push(...await boxCheck((typeof one === 'object') ? one : { sel: one })); // color: the pixels hgTracks drew in a row, which no other check here can see. A list // is allowed, and every entry is checked, so one step can state the whole of a color // matrix and a failure names every row that came out wrong rather than only the first. for (const one of (o.color == null ? [] : (Array.isArray(o.color) ? o.color : [o.color]))) { const c = (typeof one === 'object') ? one : { is: one }; const where = `${c.track}${(c.part === 'label' || c.part === 'center') ? "'s center label" : ''}`; if (!c.track) bad.push('color: needs a track'); else if (c.is == null && c.not == null) bad.push('color: needs is: or not:'); else { const got = await rowColors(c).catch(e => ({ err: e.message })); const show = g => g.top.slice(0, 3) .map(t => `${t.c.join(',')} (${Math.round(100 * t.n / g.total)}%)`).join(', '); if (got.err) bad.push(`color: ${got.err}`); // An empty row is the failure mode to name explicitly. A track that drew nothing // has no color at all, and a check that quietly passed on it -- or failed saying